User Update Internal access api - service-to-service calls#872
User Update Internal access api - service-to-service calls#872komalm wants to merge 1 commit intoELEVATE-Project:developfrom
Conversation
WalkthroughConfiguration and middleware updates to support internal service-to-service authentication. Adds INTERNAL_ACCESS_TOKEN environment variable and header constant, extends internal URL paths, and refactors authenticator middleware to handle variable URL depths and extract user/tenant information from internal request headers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client<br/>(Internal Service)
participant Auth as Authenticator<br/>Middleware
participant Headers as Request<br/>Headers
participant UserSvc as User Service/<br/>Lookup
participant Req as Request<br/>Object
Client->>Auth: Internal request to<br/>/user/v1/user/update
Auth->>Auth: Detect internal URL path
Auth->>Headers: Extract userId &<br/>tenantCode headers
alt Headers Valid
Headers-->>Auth: userId, tenantCode
Auth->>UserSvc: Load user &<br/>organization data
alt User Found & Has Org
UserSvc-->>Auth: User & Org data
Auth->>Req: Populate<br/>req.decodedToken
Auth->>Client: Proceed to handler
else User Not Found or Missing Org
UserSvc-->>Auth: Not found / error
Auth->>Client: Return error response
end
else Missing Headers
Headers-->>Auth: null/undefined
Auth->>Client: Return error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/middlewares/authenticator.js (1)
80-88:⚠️ Potential issue | 🔴 Critical
req.path.includes(path)is a substring match — this can match unintended routes.For example, a request to
/user/v1/user/update-passwordwould match the internal-access URL/user/v1/user/update, granting internal access to an endpoint that shouldn't be internally accessible. Use strict equality or a startsWith + boundary check instead.Proposed fix: use exact match or prefix with path-boundary
const internalAccess = common.internalAccessUrls.some((path) => { - if (req.path.includes(path)) { + if (req.path === path || req.path.startsWith(path + '/')) { if (req.headers.internal_access_token === process.env.INTERNAL_ACCESS_TOKEN) return true else if (!authHeader) { throw unAuthorizedResponse } } return false })As per coding guidelines,
src/middlewares/**: "This is security-sensitive middleware. Scrutinize for potential vulnerabilities."
🤖 Fix all issues with AI agents
In `@src/constants/common.js`:
- Around line 17-22: Remove the privileged endpoint from the internal access
list and harden the token bypass check: remove '/user/v1/user/update' from
internalAccessUrls (and keep INTERNAL_USER_ID_HEADER unchanged), then fix the
token validation logic in envVariables.js so it does not rely on a
truthy/undefined comparison (the current "undefined === undefined" style check)
— instead validate the presence and value of the expected env var/token
explicitly and only allow requests into internalAccessUrls when that validation
succeeds; once the token validation is corrected, you may re-add
'/user/v1/user/update' if truly internal-only.
In `@src/envVariables.js`:
- Around line 119-123: The INTERNAL_ACCESS_TOKEN env spec is marked optional
which allows undefined === undefined to pass the internal access check; update
either the env spec to required by setting INTERNAL_ACCESS_TOKEN.optional =
false in src/envVariables.js or (safer) add an explicit guard in the
authenticator middleware (authenticator.js) so the check becomes something like:
ensure process.env.INTERNAL_ACCESS_TOKEN is truthy and compare
req.headers.internal_access_token === process.env.INTERNAL_ACCESS_TOKEN, and
reject internal access when the env var is missing; reference the symbols
INTERNAL_ACCESS_TOKEN, process.env.INTERNAL_ACCESS_TOKEN and
req.headers.internal_access_token to locate the change.
In `@src/middlewares/authenticator.js`:
- Around line 116-162: The internal-access update flow improperly matches paths,
redundantly reads tenant header, lacks userId validation, sets session_id to
null, and bypasses permission checks; fix by changing the path match for
internalUserUpdatePath to exact equality or a strict prefix check (use req.path
=== internalUserUpdatePath || req.path.startsWith(internalUserUpdatePath +
'/')), remove the redundant fallback that references 'x-tenant-code' since
common.TENANT_CODE_HEADER already normalizes it, validate the userId header
(parse/validate as the expected type—integer or UUID) before calling
userQueries.findUserWithOrganization to avoid injection/coercion, replace
req.decodedToken.session_id = null with either a documented placeholder value or
a clearly commented invariant provider (or set to undefined) in
req.decodedToken, and ensure a permission check or explicit superuser assertion
is performed (or documented) before returning next() so the internal path does
not silently bypass checkPermissions.
| // Header used by data-pipeline / internal services to specify target user for internal user update (no user auth token) | ||
| INTERNAL_USER_ID_HEADER: 'x-internal-user-id', | ||
| internalAccessUrls: [ | ||
| '/user/v1/profile/details', | ||
| '/user/v1/user/profileById', | ||
| '/user/v1/user/update', |
There was a problem hiding this comment.
Exposing /user/v1/user/update as an internal-access URL is a significant privilege escalation surface — ensure the token bypass issue is fixed first.
This endpoint allows modifying user data. Combined with the undefined === undefined bypass noted in envVariables.js, any caller can hit this endpoint without authentication if the env var is unset. The constants themselves are fine once the token validation is hardened.
🤖 Prompt for AI Agents
In `@src/constants/common.js` around lines 17 - 22, Remove the privileged endpoint
from the internal access list and harden the token bypass check: remove
'/user/v1/user/update' from internalAccessUrls (and keep INTERNAL_USER_ID_HEADER
unchanged), then fix the token validation logic in envVariables.js so it does
not rely on a truthy/undefined comparison (the current "undefined === undefined"
style check) — instead validate the presence and value of the expected env
var/token explicitly and only allow requests into internalAccessUrls when that
validation succeeds; once the token validation is corrected, you may re-add
'/user/v1/user/update' if truly internal-only.
| INTERNAL_ACCESS_TOKEN: { | ||
| message: | ||
| 'Secret token for internal/service-to-service and data-pipeline API calls (e.g. user update without user auth token). Keep secure.', | ||
| optional: true, | ||
| }, |
There was a problem hiding this comment.
Critical: optional: true with no default allows internal-access bypass when the env var is unset.
In authenticator.js Line 82, the check is req.headers.internal_access_token === process.env.INTERNAL_ACCESS_TOKEN. If INTERNAL_ACCESS_TOKEN is not configured, both sides are undefined, and undefined === undefined is true — any request without the header will pass the internal access gate.
Either make this variable required (optional: false), or add an explicit guard in the authenticator that rejects internal access when the token is not configured (e.g., process.env.INTERNAL_ACCESS_TOKEN && req.headers.internal_access_token === process.env.INTERNAL_ACCESS_TOKEN).
Option A – make it required
INTERNAL_ACCESS_TOKEN: {
message:
'Secret token for internal/service-to-service and data-pipeline API calls (e.g. user update without user auth token). Keep secure.',
- optional: true,
+ optional: false,
},Option B – guard in the authenticator (src/middlewares/authenticator.js)
- if (req.headers.internal_access_token === process.env.INTERNAL_ACCESS_TOKEN) return true
+ if (process.env.INTERNAL_ACCESS_TOKEN && req.headers.internal_access_token === process.env.INTERNAL_ACCESS_TOKEN) return true🤖 Prompt for AI Agents
In `@src/envVariables.js` around lines 119 - 123, The INTERNAL_ACCESS_TOKEN env
spec is marked optional which allows undefined === undefined to pass the
internal access check; update either the env spec to required by setting
INTERNAL_ACCESS_TOKEN.optional = false in src/envVariables.js or (safer) add an
explicit guard in the authenticator middleware (authenticator.js) so the check
becomes something like: ensure process.env.INTERNAL_ACCESS_TOKEN is truthy and
compare req.headers.internal_access_token === process.env.INTERNAL_ACCESS_TOKEN,
and reject internal access when the env var is missing; reference the symbols
INTERNAL_ACCESS_TOKEN, process.env.INTERNAL_ACCESS_TOKEN and
req.headers.internal_access_token to locate the change.
| // Internal access without user token (existing internal URLs unchanged; pipeline uses internal_access_token only) | ||
| if (internalAccess && !authHeader) { | ||
| // New path only: data-pipeline user update — resolve target user from headers, set decodedToken; no change to other internal URLs | ||
| const internalUserUpdatePath = '/user/v1/user/update' | ||
| if (req.path.includes(internalUserUpdatePath)) { | ||
| // Express normalizes headers to lowercase | ||
| const userId = (req.headers[common.INTERNAL_USER_ID_HEADER] || '').trim() | ||
| const tenantCode = (req.headers[common.TENANT_CODE_HEADER] || req.headers['x-tenant-code'] || '').trim() | ||
| if (!userId || !tenantCode) { | ||
| throw responses.failureResponse({ | ||
| message: 'INTERNAL_UPDATE_REQUIRES_USER_ID_AND_TENANT', | ||
| statusCode: httpStatusCode.bad_request, | ||
| responseCode: 'CLIENT_ERROR', | ||
| }) | ||
| } | ||
| const user = await userQueries.findUserWithOrganization( | ||
| { id: userId, tenant_code: tenantCode }, | ||
| {}, | ||
| false | ||
| ) | ||
| if (!user) { | ||
| throw responses.failureResponse({ | ||
| message: 'USER_NOT_FOUND', | ||
| statusCode: httpStatusCode.unauthorized, | ||
| responseCode: 'UNAUTHORIZED', | ||
| }) | ||
| } | ||
| const org = user.organizations?.[0] | ||
| if (!org) { | ||
| throw responses.failureResponse({ | ||
| message: 'USER_HAS_NO_ORGANIZATION', | ||
| statusCode: httpStatusCode.bad_request, | ||
| responseCode: 'CLIENT_ERROR', | ||
| }) | ||
| } | ||
| req.decodedToken = { | ||
| id: user.id, | ||
| name: user.name, | ||
| session_id: null, | ||
| tenant_code: user.tenant_code, | ||
| organization_id: org.id, | ||
| organization_code: org.code, | ||
| roles: org.roles || [], | ||
| } | ||
| } | ||
| return next() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the authenticator.js file around the specified lines
fd -t f "authenticator.js" | head -5Repository: ELEVATE-Project/user
Length of output: 96
🏁 Script executed:
# Check the file structure and get context around lines 116-162
cat -n src/middlewares/authenticator.js | sed -n '110,170p'Repository: ELEVATE-Project/user
Length of output: 2556
🏁 Script executed:
# Look for common.TENANT_CODE_HEADER definition
rg -n "TENANT_CODE_HEADER" --type=js -B2 -A2Repository: ELEVATE-Project/user
Length of output: 4617
🏁 Script executed:
# Check session_id usage around line 239 and in general
rg -n "session_id" src/middlewares/authenticator.js -B2 -A2Repository: ELEVATE-Project/user
Length of output: 620
🏁 Script executed:
# Find and examine findUserWithOrganization function
fd -t f "*.js" | xargs rg -l "findUserWithOrganization" | head -5Repository: ELEVATE-Project/user
Length of output: 490
🏁 Script executed:
# Check for permission checks in the middleware
rg -n "checkPermission" src/middlewares/authenticator.js -B3 -A3Repository: ELEVATE-Project/user
Length of output: 855
🏁 Script executed:
# Find and examine findUserWithOrganization function definition
rg -n "findUserWithOrganization" -A 10 --type=js | head -40Repository: ELEVATE-Project/user
Length of output: 2366
🏁 Script executed:
# Check how userId/id is typically validated elsewhere in the codebase
rg -n "parseInt.*userId|UUID.*userId|validate.*userId" -B2 -A2 --type=js | head -50Repository: ELEVATE-Project/user
Length of output: 46
🏁 Script executed:
# Look at the userQueries to see the function signature
find src -name "*queries*" -o -name "*user*" | grep -i queryRepository: ELEVATE-Project/user
Length of output: 46
🏁 Script executed:
# Check context around line 239 to understand if the internal path actually reaches that code
cat -n src/middlewares/authenticator.js | sed -n '225,245p'Repository: ELEVATE-Project/user
Length of output: 861
🏁 Script executed:
# Verify if common.INTERNAL_USER_ID_HEADER is defined and what it contains
rg -n "INTERNAL_USER_ID_HEADER" --type=js -B2 -A2Repository: ELEVATE-Project/user
Length of output: 951
🏁 Script executed:
# Look at the userQueries file to see findUserWithOrganization implementation
find src -path "*database*" -name "*.js" | xargs rg -l "findUserWithOrganization" 2>/dev/null | head -3Repository: ELEVATE-Project/user
Length of output: 93
🏁 Script executed:
# Check database queries to see how id is used in findUserWithOrganization
rg -n "findUserWithOrganization" -A 15 src/database/queries/user.js 2>/dev/null | head -50Repository: ELEVATE-Project/user
Length of output: 46
🏁 Script executed:
# Check if there are other places where userId from headers is validated
rg -n "INTERNAL_USER_ID_HEADER\|x-internal-user-id" -B5 -A5 --type=jsRepository: ELEVATE-Project/user
Length of output: 46
🏁 Script executed:
# Verify the control flow - does the internal path actually reach line 239?
cat -n src/middlewares/authenticator.js | sed -n '115,165p'Repository: ELEVATE-Project/user
Length of output: 2118
🏁 Script executed:
# Try to find and read the users.js file in database queries
find src -name "users.js" -o -name "user.js" | grep -i databaseRepository: ELEVATE-Project/user
Length of output: 122
🏁 Script executed:
# Get the actual findUserWithOrganization implementation
rg -n "findUserWithOrganization\s*=" src/database/queries/users.js -A 20 2>/dev/null || rg -n "findUserWithOrganization" src/database/queries/users.js -A 20Repository: ELEVATE-Project/user
Length of output: 757
🏁 Script executed:
# Check if there's validation elsewhere in the middleware or at the entry point for userId
rg -n "INTERNAL_USER_ID_HEADER" -B10 -A10 --type=jsRepository: ELEVATE-Project/user
Length of output: 3059
🏁 Script executed:
# Verify the complete control flow - check if line 239 is reachable from the internal path
cat -n src/middlewares/authenticator.js | sed -n '115,250p'Repository: ELEVATE-Project/user
Length of output: 5581
🏁 Script executed:
# Verify if /user/v1/user/update-profile or similar would match req.path.includes()
node <<'EOF'
const path1 = '/user/v1/user/update';
const path2 = '/user/v1/user/update';
const path3 = '/user/v1/user/update-profile';
const path4 = '/user/v1/user/update/extra';
console.log('path.includes test:');
console.log(`path2 includes path1: ${path2.includes(path1)}`);
console.log(`path3 includes path1: ${path3.includes(path1)}`);
console.log(`path4 includes path1: ${path4.includes(path1)}`);
EOFRepository: ELEVATE-Project/user
Length of output: 164
🏁 Script executed:
# Check what happens if userId is invalid - look at Sequelize's behavior with where clause
rg -n "where:" src/database/queries/users.js -A 5 | head -20Repository: ELEVATE-Project/user
Length of output: 390
🏁 Script executed:
# Confirm that the internal path returns before reaching line 239 by checking the flow
cat -n src/middlewares/authenticator.js | sed -n '160,165p'Repository: ELEVATE-Project/user
Length of output: 162
Multiple security concerns in the internal-access update flow.
-
Line 120 – unintended path matches:
req.path.includes(internalUserUpdatePath)matches/user/v1/user/update-profileand similar paths. Use exact comparison:req.path === internalUserUpdatePath || req.path.startsWith(internalUserUpdatePath + '/'). -
Line 123 – redundant fallback:
common.TENANT_CODE_HEADERalready resolves to'x-tenant-code'by default, so the|| req.headers['x-tenant-code']fallback is a no-op. Remove it. -
Line 122 – no userId validation:
userIdfrom the header is passed directly to the database query without type checking. Validate it as a valid integer or UUID to prevent Sequelize type coercion issues or unexpected query behavior. -
Line 154 – null session_id in decodedToken: While the internal path returns before reaching downstream code that accesses
session_id(line 239), explicitly null values in token objects complicate future refactoring. Either set a valid placeholder or document the invariant clearly. -
Lines 116–162 – no permission check: The internal update path bypasses
checkPermissionsentirely. If this internal token is meant to act as a superuser credential, document the security implications explicitly.
Proposed fixes for items 1–3
- if (req.path.includes(internalUserUpdatePath)) {
+ if (req.path === internalUserUpdatePath || req.path.startsWith(internalUserUpdatePath + '/')) {
// Express normalizes headers to lowercase
const userId = (req.headers[common.INTERNAL_USER_ID_HEADER] || '').trim()
- const tenantCode = (req.headers[common.TENANT_CODE_HEADER] || req.headers['x-tenant-code'] || '').trim()
- if (!userId || !tenantCode) {
+ const tenantCode = (req.headers[common.TENANT_CODE_HEADER] || '').trim()
+ if (!userId || !tenantCode || !/^\d+$/.test(userId)) {
throw responses.failureResponse({
message: 'INTERNAL_UPDATE_REQUIRES_USER_ID_AND_TENANT',
statusCode: httpStatusCode.bad_request,
responseCode: 'CLIENT_ERROR',
})
}🤖 Prompt for AI Agents
In `@src/middlewares/authenticator.js` around lines 116 - 162, The internal-access
update flow improperly matches paths, redundantly reads tenant header, lacks
userId validation, sets session_id to null, and bypasses permission checks; fix
by changing the path match for internalUserUpdatePath to exact equality or a
strict prefix check (use req.path === internalUserUpdatePath ||
req.path.startsWith(internalUserUpdatePath + '/')), remove the redundant
fallback that references 'x-tenant-code' since common.TENANT_CODE_HEADER already
normalizes it, validate the userId header (parse/validate as the expected
type—integer or UUID) before calling userQueries.findUserWithOrganization to
avoid injection/coercion, replace req.decodedToken.session_id = null with either
a documented placeholder value or a clearly commented invariant provider (or set
to undefined) in req.decodedToken, and ensure a permission check or explicit
superuser assertion is performed (or documented) before returning next() so the
internal path does not silently bypass checkPermissions.
1 participant
Summary by CodeRabbit