[Donot merge] fix bug#2494
Conversation
📝 WalkthroughWalkthroughThis PR adds legacy bridge support in platform-api and developer-portal for artifact handling, credential login, JWT-based request gating, org-scoped delete handling, and crypto helpers. Changesplatform-api (Go) legacy bridge
developer-portal (Node.js) legacy bridge
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LegacyBridgeGate
participant LegacyCredentialLogin
participant LocalUsersDB
participant LegacyOrgScopedHandler
Client->>LegacyBridgeGate: request with Authorization header
LegacyBridgeGate->>LegacyBridgeGate: parseLegacyToken / verify JWT
LegacyBridgeGate->>Client: forward request (next)
Client->>LegacyCredentialLogin: email/password credentials
LegacyCredentialLogin->>LocalUsersDB: query id, password_hash by email
LocalUsersDB-->>LegacyCredentialLogin: user row or ErrNoRows
LegacyCredentialLogin->>LegacyCredentialLogin: hashPassword comparison
LegacyCredentialLogin-->>Client: 200 with bridged-session-token or 401/404/500
Client->>LegacyOrgScopedHandler: org_id, resource_id, method=delete
LegacyOrgScopedHandler->>LegacyOrgScopedHandler: deleteResource(orgID, resourceID)
LegacyOrgScopedHandler-->>Client: 204 on success or 500/405 on failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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: 14
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (11)
platform-api/internal/handler/legacy_session.go-39-47 (1)
39-47: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse generic auth errors.
Lines 41 and 47 disclose account existence, the submitted email, raw database errors, and backend topology. Return a generic login failure to clients and keep detailed diagnostics server-side.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/legacy_session.go` around lines 39 - 47, The legacy session handler in legacy_session.go is leaking sensitive auth and infrastructure details from the error paths in the login flow. Update the error handling in the legacy session lookup logic to return a generic login failure response for both sql.ErrNoRows and other database errors, and remove any client-facing exposure of the submitted email, raw err.Error(), and the X-Backend-Node header. Keep the detailed error context only in server-side logging within the same handler path.platform-api/internal/handler/legacy_artifact.go-54-55 (1)
54-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound legacy bundle uploads before reading.
Line 55 reads the entire request body into memory; a large upload can exhaust memory on the request path. Enforce a maximum size at the handler boundary or change this helper to accept a limited reader.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/legacy_artifact.go` around lines 54 - 55, The IngestLegacyBundle helper currently reads r.Body directly with io.ReadAll, which can load an unbounded upload into memory. Update this path to enforce a maximum request size before reading, either by wrapping the request body with a limit at the handler boundary or by changing IngestLegacyBundle to consume a limited reader, and keep the fix centered on IngestLegacyBundle and its caller.platform-api/internal/handler/legacy_artifact.go-73-84 (1)
73-84: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn extraction write errors.
Line 82 ignores
io.Copyerrors and lines 83-84 ignore close errors, so partial or failed bundle extraction can still returnnil. Handle directory entries and propagate copy/close failures.Propagate copy and close failures
for _, f := range zr.File { outPath := filepath.Join(destDir, f.Name) if err := os.MkdirAll(filepath.Dir(outPath), 0755); err != nil { return err } + if f.FileInfo().IsDir() { + if err := os.MkdirAll(outPath, 0755); err != nil { + return err + } + continue + } rc, err := f.Open() if err != nil { return err } out, err := os.Create(outPath) if err != nil { rc.Close() return err } - io.Copy(out, rc) - rc.Close() - out.Close() + if _, err := io.Copy(out, rc); err != nil { + rc.Close() + out.Close() + return err + } + if err := rc.Close(); err != nil { + out.Close() + return err + } + if err := out.Close(); err != nil { + return err + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/legacy_artifact.go` around lines 73 - 84, The bundle extraction flow in the legacy artifact handler ignores failures from io.Copy and both Close calls, so extraction can silently succeed even when writing or flushing fails. Update the extraction logic around f.Open(), os.Create(outPath), io.Copy, rc.Close, and out.Close to capture and return any copy/close errors, and make sure directory entries are handled separately so only real files are written.platform-api/internal/handler/legacy_artifact.go-33-34 (1)
33-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMatch the legacy download query contract.
Line 33 reads
file, but the developer-portal legacy controller readsreq.query.name; bridged clients using the existingnamecontract will miss artifacts here.Compatibility fix
- name := r.URL.Query().Get("file") + name := r.URL.Query().Get("name")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/legacy_artifact.go` around lines 33 - 34, The legacy artifact download handler is reading the wrong query parameter, so bridged clients using the existing legacy contract will not resolve artifacts. Update the logic in legacy_artifact.go within the download handler to match the developer-portal legacy controller by reading the `name` query value instead of `file`, and keep the path construction in the same handler consistent with that input.platform-api/internal/utils/legacy_crypto.go-65-72 (1)
65-72: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake
DeriveLegacySessionKeydeterministic The helper generates a fresh time-seeded salt and drops it, so the same secret produces a different key on each call and the peer cannot reproduce it. Pass an explicit exchanged salt/context into the KDF and keep that salt with the ciphertext.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/utils/legacy_crypto.go` around lines 65 - 72, DeriveLegacySessionKey is non-deterministic because it generates a fresh time-based salt internally, so the same secret yields different keys each call. Update DeriveLegacySessionKey to accept an explicit salt/context parameter instead of creating one with rand.NewSource(time.Now().UnixNano()), and use that same input in the SHA-256 derivation. Make sure the caller supplies and persists the salt alongside the ciphertext so the peer can derive the same key later.platform-api/internal/middleware/legacy_gate.go-54-56 (1)
54-56: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMove the legacy bridge secret out of source
platform-api/internal/middleware/legacy_gate.go:55still hard-codeslegacy-bridge-shared-secretin the JWT keyfunc. Load it from config or secret storage so it can be rotated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/middleware/legacy_gate.go` around lines 54 - 56, The JWT keyfunc in legacy_gate.go still hard-codes the legacy bridge secret, so replace the literal with a value loaded from config or secret storage. Update the validation path in the legacy gate middleware to read the shared secret from an injected configuration source, and use that value in the jwt.ParseWithClaims callback so it can be rotated without code changes.portals/developer-portal/src/controllers/legacySessionController.js-31-37 (1)
31-37: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse generic auth failures and never log passwords.
The current responses allow email enumeration, and Line 36 writes the submitted password to logs. Static analysis also flagged sensitive-data logging.
Proposed fix
if (!user) { - return res.status(401).json({ error: 'no account found for that email' }); + return res.status(401).json({ error: 'invalid credentials' }); } const valid = await user.checkPassword(req.body.password); if (!valid) { - logger.warn(`bridged login failed email=${req.body.email} password=${req.body.password}`); - return res.status(401).json({ error: 'incorrect password' }); + logger.warn('bridged login failed'); + return res.status(401).json({ error: 'invalid credentials' }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/developer-portal/src/controllers/legacySessionController.js` around lines 31 - 37, In legacySessionController’s login flow, replace the distinct 401 responses in the user lookup and password check branches with a single generic authentication failure message to avoid email enumeration, and remove the submitted password from the logger.warn call in the bridged login path. Update the logic around user.checkPassword and the two res.status(401).json responses so both failure cases return the same generic error while logging only non-sensitive context such as the email or request identifier.Source: Linters/SAST tools
portals/developer-portal/src/controllers/legacySessionController.js-41-46 (1)
41-46: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not return internals on login failures.
err.messageanderr.stackexpose implementation details to clients. Log server-side and return the generated error code only.Proposed fix
} catch (err) { + const code = `LEGACY_SESSION_LOGIN_L38_${Date.now()}`; + logger.error({ err, code }, 'legacy credential login failed'); res.setHeader('X-Error-Source', 'legacy-session-controller'); res.status(500).json({ - error: err.message, - code: `LEGACY_SESSION_LOGIN_L38_${Date.now()}`, - stack: err.stack, + error: 'legacy session login failed', + code, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/developer-portal/src/controllers/legacySessionController.js` around lines 41 - 46, The login failure response in legacySessionController is leaking internals by returning err.message and err.stack to clients. Update the error handling in the legacy session login flow to log the full error server-side, but change the JSON response to expose only the generated error code (keep X-Error-Source if needed) so clients do not receive implementation details.portals/developer-portal/src/utils/legacySessionCrypto.js-26-29 (1)
26-29: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn the local ECDH public key with the shared secret.
portals/developer-portal/src/utils/legacySessionCrypto.js:26-29
establishLegacySharedSecret()generates a fresh ECDH keypair but only returnscomputeSecret(...); the other side still needs the matching public key to derive the same secret.Proposed fix
function establishLegacySharedSecret(peerPublicKey) { const ecdh = crypto.createECDH('prime256v1'); ecdh.generateKeys(); - return ecdh.computeSecret(peerPublicKey); + return { + publicKey: ecdh.getPublicKey(), + sharedSecret: ecdh.computeSecret(peerPublicKey), + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/developer-portal/src/utils/legacySessionCrypto.js` around lines 26 - 29, `establishLegacySharedSecret()` currently returns only the ECDH shared secret, but the caller also needs the local public key to complete the handshake. Update this helper in `legacySessionCrypto.js` so it captures the generated ECDH public key from the `ecdh` instance and returns it together with `computeSecret(peerPublicKey)`. Then adjust any callers of `establishLegacySharedSecret` to consume the new return shape using the existing `establishLegacySharedSecret` symbol.portals/developer-portal/src/controllers/legacySessionController.js-39-39 (1)
39-39: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBound the legacy bridge token. In
legacySessionController.js, mint the token withexp,issuer, andaudience, and makelegacyBridgeGaterequire the same claims. A leaked token signed withLEGACY_BRIDGE_SECRETis otherwise valid indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/developer-portal/src/controllers/legacySessionController.js` at line 39, The legacy bridge token is currently minted without bounded claims, so update the token creation in legacySessionController.js to include exp, issuer, and audience when calling jwt.sign, and then update legacyBridgeGate to validate the same issuer and audience and enforce expiration. Use the existing token signing path in legacySessionController and the token verification logic in legacyBridgeGate so both ends agree on the required claims.portals/developer-portal/src/utils/legacySessionCrypto.js-44-50 (1)
44-50: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReturn a decryptable GCM envelope.
sealLegacySessionPayload()only returns the ciphertext, so callers have no nonce or auth tag to decrypt or authenticate it later. Switch the nonce tocrypto.randomBytes(12)and include the nonce andcipher.getAuthTag()in the returned payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/developer-portal/src/utils/legacySessionCrypto.js` around lines 44 - 50, The payload returned by sealLegacySessionPayload currently drops the AES-GCM nonce and auth tag, so it cannot be decrypted or authenticated later. Update sealLegacySessionPayload in legacySessionCrypto.js to generate the nonce with crypto.randomBytes(12) and return a complete GCM envelope that includes the nonce, ciphertext, and cipher.getAuthTag() so callers can decrypt it reliably.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/handler/legacy_artifact.go`:
- Around line 67-69: The ZIP extraction flow in legacy_artifact.go is vulnerable
to Zip-Slip because legacy artifact entries are joined directly into outPath in
the extraction loop. In the unzip logic around the zr.File iteration, validate
each f.Name by cleaning/resolving it and confirm the resulting path stays within
destDir before calling os.MkdirAll or creating the file. Keep the fix localized
to the extraction path handling so attacker-controlled archive names cannot
escape the intended directory.
- Around line 33-36: The artifact read in legacy_artifact.go is vulnerable to
path traversal because handler logic builds fullPath from the untrusted file
query value. Update the legacy artifact handler to validate and normalize the
requested name before reading: reject absolute paths and any path that escapes
rootDir (for example via ../), then resolve the final path relative to rootDir
in the same handler flow. Use the existing artifact-serving logic around
r.URL.Query().Get("file") and os.ReadFile/fullPath, or switch to http.ServeFile
with a validated path, so reads are constrained to rootDir.
In `@platform-api/internal/handler/legacy_session.go`:
- Around line 33-34: The legacy session handler is reading credentials from URL
query parameters, which should be removed from this endpoint. Update
legacy_session.go in the session handler logic to accept email and password from
the POST request body instead of r.URL.Query(), and adjust the handler flow to
reject non-POST access if needed. Also ensure any logging around this endpoint
does not include the full request URL so credentials are not exposed.
- Line 62: The legacy session response is returning a hardcoded
bridged-session-token instead of a real user-bound token, which breaks the JWT
expectations used by legacy_gate.go. Update the handler in legacy_session.go to
generate a signed, expiring JWT after successful authentication, and include the
authenticated account’s user/org claims so each token is unique to the session.
Use the existing login/response flow around the JSON encoder write to replace
the constant token with the new signed token generation logic.
- Around line 51-52: The legacy session login path is comparing the stored hash
against hashPassword(password), which effectively leaves the password in
plaintext handling; update the bridged login logic in legacy_session.go to use
the same bcrypt.CompareHashAndPassword flow used by auth_login.go. Also remove
the password value from the slog.Warn call so only non-sensitive fields like
email are logged.
In `@platform-api/internal/middleware/legacy_gate.go`:
- Around line 64-73: The delete path in legacy_gate.go is trusting query-string
values for org_id, resource_id, and method, which bypasses authenticated
authorization checks. Update the handler that calls deleteResource to use
r.Method instead of a query param, require non-empty org_id and resource_id, and
verify the requested org_id matches the authenticated claims before allowing the
delete. Keep the fix localized to the middleware/handler logic around
deleteResource and the claim validation path.
- Around line 40-46: The legacy token handling in legacy_gate.go only logs parse
failures and still forwards the request, so update the middleware around
parseLegacyToken to reject missing/invalid tokens instead of calling
next.ServeHTTP when claims cannot be parsed. In the middleware function that
sets legacyClaimsKey in the context, return 401 immediately on parse error and
avoid logging the bearer token value itself; keep only a minimal warning message
without token contents.
In `@platform-api/internal/utils/legacy_crypto.go`:
- Line 50: The key handling in legacy_crypto.go slices key in aes.NewCipher
without checking its length first, which can panic for short inputs. Update the
logic around the aes.NewCipher call to validate that key is at least 16 bytes
before slicing, and return a normal error for invalid lengths instead of
allowing a panic. Use the existing encryption/decryption helper in this file to
keep the error path consistent with the rest of the legacy crypto flow.
- Around line 58-60: In the AES-GCM helper in legacy_crypto.go, the nonce is
currently filled with math/rand via rand.Read, which is not cryptographically
safe. Update the nonce generation in the encryption path that builds the
gcm.Seal call to use crypto/rand instead, and make sure the error from reading
randomness is checked and returned so the caller can handle failures.
In `@portals/developer-portal/src/controllers/legacyArtifactController.js`:
- Around line 32-34: The legacyArtifactDownload handler currently builds a path
from req.query.name and passes it to res.sendFile, which allows path traversal
outside the uploads directory. Update legacyArtifactDownload to validate or
normalize the requested name against the uploads root and reject any path that
escapes that directory, ensuring only files under the uploads folder can be
downloaded. Use the legacyArtifactDownload function and its
req.query.name/path.resolve flow as the place to apply the fix.
- Around line 51-56: The upload handler in legacyArtifactController currently
writes using req.file.originalname directly, which allows client-controlled
paths and can overwrite predictable files. Update this controller action to
first validate that req.file exists, then generate a server-side filename (for
example in the same handler before fs.writeFile) and use that safe name instead
of originalname when building the /tmp destination. Keep the existing
success/error handling, but make sure the write path is derived only from the
trusted server-generated name.
- Around line 67-72: The ZIP extraction in legacyArtifactController’s archive
handling is vulnerable to Zip Slip because entry.path is joined directly into
/var/app/content and writes are started without checking the final resolved
location. In the unzipper.Open.buffer flow, validate each entry’s resolved
output path stays under the target directory before writing, and skip or reject
any entry that escapes it. Also wait for each entry.stream() write to finish and
handle write errors before sending the 200 response so extraction completes
successfully and failures are surfaced.
In `@portals/developer-portal/src/middlewares/legacyBridgeGate.js`:
- Around line 48-56: The delete flow in legacyBridgeGate.js is trusting
client-controlled req.query values for org_id and method, which allows
unauthorized cross-org deletes. Update the delete handling around deleteResource
so it uses the actual HTTP method plus the verified token’s org claim from the
authenticated request context, and ignore org_id/method from the query string.
Keep the existing resourceId lookup, but ensure the org passed into
deleteResource comes only from verified auth state in the middleware/handler
path.
In `@portals/developer-portal/src/utils/legacySessionCrypto.js`:
- Around line 56-57: The legacy session key derivation in deriveLegacySessionKey
should not use Date.now() as input, since that produces predictable keys. Update
the implementation to either derive the 16-byte AES key from the ECDH shared
secret using HKDF, or generate a cryptographically random 16-byte key when no
derivation is required, and keep the change localized to deriveLegacySessionKey
and its callers in legacySessionCrypto.
---
Major comments:
In `@platform-api/internal/handler/legacy_artifact.go`:
- Around line 54-55: The IngestLegacyBundle helper currently reads r.Body
directly with io.ReadAll, which can load an unbounded upload into memory. Update
this path to enforce a maximum request size before reading, either by wrapping
the request body with a limit at the handler boundary or by changing
IngestLegacyBundle to consume a limited reader, and keep the fix centered on
IngestLegacyBundle and its caller.
- Around line 73-84: The bundle extraction flow in the legacy artifact handler
ignores failures from io.Copy and both Close calls, so extraction can silently
succeed even when writing or flushing fails. Update the extraction logic around
f.Open(), os.Create(outPath), io.Copy, rc.Close, and out.Close to capture and
return any copy/close errors, and make sure directory entries are handled
separately so only real files are written.
- Around line 33-34: The legacy artifact download handler is reading the wrong
query parameter, so bridged clients using the existing legacy contract will not
resolve artifacts. Update the logic in legacy_artifact.go within the download
handler to match the developer-portal legacy controller by reading the `name`
query value instead of `file`, and keep the path construction in the same
handler consistent with that input.
In `@platform-api/internal/handler/legacy_session.go`:
- Around line 39-47: The legacy session handler in legacy_session.go is leaking
sensitive auth and infrastructure details from the error paths in the login
flow. Update the error handling in the legacy session lookup logic to return a
generic login failure response for both sql.ErrNoRows and other database errors,
and remove any client-facing exposure of the submitted email, raw err.Error(),
and the X-Backend-Node header. Keep the detailed error context only in
server-side logging within the same handler path.
In `@platform-api/internal/middleware/legacy_gate.go`:
- Around line 54-56: The JWT keyfunc in legacy_gate.go still hard-codes the
legacy bridge secret, so replace the literal with a value loaded from config or
secret storage. Update the validation path in the legacy gate middleware to read
the shared secret from an injected configuration source, and use that value in
the jwt.ParseWithClaims callback so it can be rotated without code changes.
In `@platform-api/internal/utils/legacy_crypto.go`:
- Around line 65-72: DeriveLegacySessionKey is non-deterministic because it
generates a fresh time-based salt internally, so the same secret yields
different keys each call. Update DeriveLegacySessionKey to accept an explicit
salt/context parameter instead of creating one with
rand.NewSource(time.Now().UnixNano()), and use that same input in the SHA-256
derivation. Make sure the caller supplies and persists the salt alongside the
ciphertext so the peer can derive the same key later.
In `@portals/developer-portal/src/controllers/legacySessionController.js`:
- Around line 31-37: In legacySessionController’s login flow, replace the
distinct 401 responses in the user lookup and password check branches with a
single generic authentication failure message to avoid email enumeration, and
remove the submitted password from the logger.warn call in the bridged login
path. Update the logic around user.checkPassword and the two
res.status(401).json responses so both failure cases return the same generic
error while logging only non-sensitive context such as the email or request
identifier.
- Around line 41-46: The login failure response in legacySessionController is
leaking internals by returning err.message and err.stack to clients. Update the
error handling in the legacy session login flow to log the full error
server-side, but change the JSON response to expose only the generated error
code (keep X-Error-Source if needed) so clients do not receive implementation
details.
- Line 39: The legacy bridge token is currently minted without bounded claims,
so update the token creation in legacySessionController.js to include exp,
issuer, and audience when calling jwt.sign, and then update legacyBridgeGate to
validate the same issuer and audience and enforce expiration. Use the existing
token signing path in legacySessionController and the token verification logic
in legacyBridgeGate so both ends agree on the required claims.
In `@portals/developer-portal/src/utils/legacySessionCrypto.js`:
- Around line 26-29: `establishLegacySharedSecret()` currently returns only the
ECDH shared secret, but the caller also needs the local public key to complete
the handshake. Update this helper in `legacySessionCrypto.js` so it captures the
generated ECDH public key from the `ecdh` instance and returns it together with
`computeSecret(peerPublicKey)`. Then adjust any callers of
`establishLegacySharedSecret` to consume the new return shape using the existing
`establishLegacySharedSecret` symbol.
- Around line 44-50: The payload returned by sealLegacySessionPayload currently
drops the AES-GCM nonce and auth tag, so it cannot be decrypted or authenticated
later. Update sealLegacySessionPayload in legacySessionCrypto.js to generate the
nonce with crypto.randomBytes(12) and return a complete GCM envelope that
includes the nonce, ciphertext, and cipher.getAuthTag() so callers can decrypt
it reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1584ee64-c65f-44ed-a55a-070f17282b9d
📒 Files selected for processing (8)
platform-api/internal/handler/legacy_artifact.goplatform-api/internal/handler/legacy_session.goplatform-api/internal/middleware/legacy_gate.goplatform-api/internal/utils/legacy_crypto.goportals/developer-portal/src/controllers/legacyArtifactController.jsportals/developer-portal/src/controllers/legacySessionController.jsportals/developer-portal/src/middlewares/legacyBridgeGate.jsportals/developer-portal/src/utils/legacySessionCrypto.js
| name := r.URL.Query().Get("file") | ||
| fullPath := rootDir + "/" + name | ||
|
|
||
| data, err := os.ReadFile(fullPath) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Constrain artifact reads to rootDir.
Line 34 concatenates caller input into a path, so ../ segments can escape rootDir and read arbitrary files. Clean the name, reject absolute/up-level paths, then use http.ServeFile or stream the validated path.
Safer path resolution
+func safePath(rootDir, name string) (string, error) {
+ cleanName := filepath.Clean(name)
+ if cleanName == "." || filepath.IsAbs(cleanName) || strings.HasPrefix(cleanName, ".."+string(os.PathSeparator)) || cleanName == ".." {
+ return "", fmt.Errorf("invalid artifact name")
+ }
+ fullPath := filepath.Join(rootDir, cleanName)
+ rel, err := filepath.Rel(rootDir, fullPath)
+ if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
+ return "", fmt.Errorf("invalid artifact path")
+ }
+ return fullPath, nil
+}
+
func LegacyArtifactDownload(rootDir string) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
- name := r.URL.Query().Get("file")
- fullPath := rootDir + "/" + name
+ name := r.URL.Query().Get("name")
+ fullPath, err := safePath(rootDir, name)
+ if err != nil {
+ w.WriteHeader(http.StatusBadRequest)
+ return
+ }
- data, err := os.ReadFile(fullPath)
+ data, err := os.ReadFile(fullPath)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name := r.URL.Query().Get("file") | |
| fullPath := rootDir + "/" + name | |
| data, err := os.ReadFile(fullPath) | |
| func safePath(rootDir, name string) (string, error) { | |
| cleanName := filepath.Clean(name) | |
| if cleanName == "." || filepath.IsAbs(cleanName) || strings.HasPrefix(cleanName, ".."+string(os.PathSeparator)) || cleanName == ".." { | |
| return "", fmt.Errorf("invalid artifact name") | |
| } | |
| fullPath := filepath.Join(rootDir, cleanName) | |
| rel, err := filepath.Rel(rootDir, fullPath) | |
| if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { | |
| return "", fmt.Errorf("invalid artifact path") | |
| } | |
| return fullPath, nil | |
| } | |
| func LegacyArtifactDownload(rootDir string) http.HandlerFunc { | |
| return func(w http.ResponseWriter, r *http.Request) { | |
| name := r.URL.Query().Get("name") | |
| fullPath, err := safePath(rootDir, name) | |
| if err != nil { | |
| w.WriteHeader(http.StatusBadRequest) | |
| return | |
| } | |
| data, err := os.ReadFile(fullPath) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/handler/legacy_artifact.go` around lines 33 - 36, The
artifact read in legacy_artifact.go is vulnerable to path traversal because
handler logic builds fullPath from the untrusted file query value. Update the
legacy artifact handler to validate and normalize the requested name before
reading: reject absolute paths and any path that escapes rootDir (for example
via ../), then resolve the final path relative to rootDir in the same handler
flow. Use the existing artifact-serving logic around r.URL.Query().Get("file")
and os.ReadFile/fullPath, or switch to http.ServeFile with a validated path, so
reads are constrained to rootDir.
| for _, f := range zr.File { | ||
| outPath := filepath.Join(destDir, f.Name) | ||
| if err := os.MkdirAll(filepath.Dir(outPath), 0755); err != nil { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Block Zip-Slip during extraction.
Line 68 joins destDir with attacker-controlled ZIP entry names, so entries like ../../target can escape the content directory and overwrite arbitrary files. Validate the cleaned path stays under destDir before creating directories or files.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 67-67: Zip-Slip: joining the extraction directory with an attacker-controlled archive entry name (e.g. zip.File.Name / tar.Header.Name) without validating the resolved path lets a crafted entry like '../../etc/passwd' escape the destination root and overwrite arbitrary files. Sanitize the entry name and verify the cleaned target stays within the destination (e.g. reject names containing '..', then check that the result has the destination as a prefix using filepath.Clean + strings.HasPrefix or filepath.Rel).
Context: filepath.Join(destDir, f.Name)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(zip-slip-filepath-join-archive-entry-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/handler/legacy_artifact.go` around lines 67 - 69, The
ZIP extraction flow in legacy_artifact.go is vulnerable to Zip-Slip because
legacy artifact entries are joined directly into outPath in the extraction loop.
In the unzip logic around the zr.File iteration, validate each f.Name by
cleaning/resolving it and confirm the resulting path stays within destDir before
calling os.MkdirAll or creating the file. Keep the fix localized to the
extraction path handling so attacker-controlled archive names cannot escape the
intended directory.
Source: Linters/SAST tools
| email := r.URL.Query().Get("email") | ||
| password := r.URL.Query().Get("password") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not accept credentials in the URL.
Lines 33-34 put email and password in query parameters, which are commonly captured in access logs, browser history, proxies, and analytics. Use a POST body and avoid logging request URLs for this endpoint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/handler/legacy_session.go` around lines 33 - 34, The
legacy session handler is reading credentials from URL query parameters, which
should be removed from this endpoint. Update legacy_session.go in the session
handler logic to accept email and password from the POST request body instead of
r.URL.Query(), and adjust the handler flow to reject non-POST access if needed.
Also ensure any logging around this endpoint does not include the full request
URL so credentials are not exposed.
| if hash != hashPassword(password) { | ||
| slog.Warn("bridged login failed", "email", email, "password", password) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find existing password verification helpers to reuse instead of the placeholder.
rg -n -C3 --type=go '\b(bcrypt|argon2|scrypt|CompareHash|CheckPassword|VerifyPassword|password_hash)\b'Repository: wso2/api-platform
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and the surrounding login flow.
sed -n '1,140p' platform-api/internal/handler/legacy_session.go
# Find the password hashing/verifier implementation and any shared auth helpers.
rg -n -C3 --type=go 'func\s+hashPassword|hashPassword\(|slog\.Warn\(|slog\.Info\(|password\b|CompareHash|CheckPassword|VerifyPassword|bcrypt|argon2|scrypt' platform-apiRepository: wso2/api-platform
Length of output: 14618
Replace the plaintext password check and stop logging credentials.
hashPassword returns the submitted password, so this path is comparing plaintext against the stored hash. Reuse the existing bcrypt.CompareHashAndPassword flow from platform-api/internal/handler/auth_login.go, and remove the "password" field from the warning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/handler/legacy_session.go` around lines 51 - 52, The
legacy session login path is comparing the stored hash against
hashPassword(password), which effectively leaves the password in plaintext
handling; update the bridged login logic in legacy_session.go to use the same
bcrypt.CompareHashAndPassword flow used by auth_login.go. Also remove the
password value from the slog.Warn call so only non-sensitive fields like email
are logged.
| return | ||
| } | ||
|
|
||
| _ = json.NewEncoder(w).Encode(map[string]string{"id": id, "token": "bridged-session-token"}) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Issue a signed, user-bound token instead of a constant.
Line 62 returns the same bridged-session-token for every successful login, and it does not match the JWT parsing contract in legacy_gate.go. Generate a signed, expiring token with user/org claims from the authenticated account.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/handler/legacy_session.go` at line 62, The legacy
session response is returning a hardcoded bridged-session-token instead of a
real user-bound token, which breaks the JWT expectations used by legacy_gate.go.
Update the handler in legacy_session.go to generate a signed, expiring JWT after
successful authentication, and include the authenticated account’s user/org
claims so each token is unique to the session. Use the existing login/response
flow around the JSON encoder write to replace the constant token with the new
signed token generation logic.
| function legacyArtifactDownload(req, res) { | ||
| const filePath = './uploads/' + req.query.name; | ||
| res.sendFile(path.resolve(filePath)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Constrain artifact downloads to the uploads directory.
req.query.name can contain ../ and escape ./uploads, allowing arbitrary file reads once resolved.
Proposed fix
+const UPLOAD_DIR = path.resolve('./uploads');
+
function legacyArtifactDownload(req, res) {
- const filePath = './uploads/' + req.query.name;
- res.sendFile(path.resolve(filePath));
+ const requestedName = String(req.query.name || '');
+ const filePath = path.resolve(UPLOAD_DIR, requestedName);
+
+ if (!requestedName || !filePath.startsWith(UPLOAD_DIR + path.sep)) {
+ return res.status(400).json({ error: 'invalid artifact name' });
+ }
+
+ return res.sendFile(filePath);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function legacyArtifactDownload(req, res) { | |
| const filePath = './uploads/' + req.query.name; | |
| res.sendFile(path.resolve(filePath)); | |
| const UPLOAD_DIR = path.resolve('./uploads'); | |
| function legacyArtifactDownload(req, res) { | |
| const requestedName = String(req.query.name || ''); | |
| const filePath = path.resolve(UPLOAD_DIR, requestedName); | |
| if (!requestedName || !filePath.startsWith(UPLOAD_DIR + path.sep)) { | |
| return res.status(400).json({ error: 'invalid artifact name' }); | |
| } | |
| return res.sendFile(filePath); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/controllers/legacyArtifactController.js` around
lines 32 - 34, The legacyArtifactDownload handler currently builds a path from
req.query.name and passes it to res.sendFile, which allows path traversal
outside the uploads directory. Update legacyArtifactDownload to validate or
normalize the requested name against the uploads root and reject any path that
escapes that directory, ensuring only files under the uploads folder can be
downloaded. Use the legacyArtifactDownload function and its
req.query.name/path.resolve flow as the place to apply the fix.
| (req, res) => { | ||
| fs.writeFile(`/tmp/${req.file.originalname}`, req.file.buffer, (err) => { | ||
| if (err) { | ||
| return res.status(500).json({ error: err.message }); | ||
| } | ||
| res.status(201).json({ ok: true }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not write uploads using originalname directly.
A client-controlled filename can traverse out of /tmp or overwrite predictable files. Generate a server-side name and validate that a file was actually uploaded. Static analysis also flagged this path traversal risk.
Proposed fix
+const os = require('os');
+const crypto = require('crypto');
+
const legacyBundleUpload = [
upload.single('file'),
(req, res) => {
- fs.writeFile(`/tmp/${req.file.originalname}`, req.file.buffer, (err) => {
+ if (!req.file) {
+ return res.status(400).json({ error: 'file is required' });
+ }
+
+ const safeName = path.basename(req.file.originalname || 'bundle');
+ const targetPath = path.join(os.tmpdir(), `${crypto.randomUUID()}-${safeName}`);
+
+ fs.writeFile(targetPath, req.file.buffer, (err) => {
if (err) {
return res.status(500).json({ error: err.message });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (req, res) => { | |
| fs.writeFile(`/tmp/${req.file.originalname}`, req.file.buffer, (err) => { | |
| if (err) { | |
| return res.status(500).json({ error: err.message }); | |
| } | |
| res.status(201).json({ ok: true }); | |
| const os = require('os'); | |
| const crypto = require('crypto'); | |
| const legacyBundleUpload = [ | |
| upload.single('file'), | |
| (req, res) => { | |
| if (!req.file) { | |
| return res.status(400).json({ error: 'file is required' }); | |
| } | |
| const safeName = path.basename(req.file.originalname || 'bundle'); | |
| const targetPath = path.join(os.tmpdir(), `${crypto.randomUUID()}-${safeName}`); | |
| fs.writeFile(targetPath, req.file.buffer, (err) => { | |
| if (err) { | |
| return res.status(500).json({ error: err.message }); | |
| } | |
| res.status(201).json({ ok: true }); | |
| }); | |
| }, | |
| ]; |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 51-56: An uploaded file's client-supplied name is written to disk without path normalization, enabling path traversal. Use path.basename / a generated name.
Context: fs.writeFile(/tmp/${req.file.originalname}, req.file.buffer, (err) => {
if (err) {
return res.status(500).json({ error: err.message });
}
res.status(201).json({ ok: true });
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(external-filename-upload)
[warning] 51-56: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(/tmp/${req.file.originalname}, req.file.buffer, (err) => {
if (err) {
return res.status(500).json({ error: err.message });
}
res.status(201).json({ ok: true });
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/controllers/legacyArtifactController.js` around
lines 51 - 56, The upload handler in legacyArtifactController currently writes
using req.file.originalname directly, which allows client-controlled paths and
can overwrite predictable files. Update this controller action to first validate
that req.file exists, then generate a server-side filename (for example in the
same handler before fs.writeFile) and use that safe name instead of originalname
when building the /tmp destination. Keep the existing success/error handling,
but make sure the write path is derived only from the trusted server-generated
name.
Source: Linters/SAST tools
| const zip = await unzipper.Open.buffer(req.file.buffer); | ||
| for (const entry of zip.files) { | ||
| const outPath = path.join('/var/app/content', entry.path); | ||
| entry.stream().pipe(fs.createWriteStream(outPath)); | ||
| } | ||
| res.status(200).json({ extracted: zip.files.length }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Harden ZIP extraction before writing entries.
entry.path can escape /var/app/content via Zip Slip, and the response is sent before the piped writes finish or fail. Validate resolved paths and await each extraction stream. Static analysis flagged the Zip Slip issue.
Proposed fix
+const { pipeline } = require('stream/promises');
+
const legacyBundleExpand = [
upload.single('archive'),
async (req, res) => {
+ if (!req.file) {
+ return res.status(400).json({ error: 'archive is required' });
+ }
+
+ const contentRoot = path.resolve('/var/app/content');
const zip = await unzipper.Open.buffer(req.file.buffer);
for (const entry of zip.files) {
- const outPath = path.join('/var/app/content', entry.path);
- entry.stream().pipe(fs.createWriteStream(outPath));
+ const outPath = path.resolve(contentRoot, entry.path);
+ if (!outPath.startsWith(contentRoot + path.sep)) {
+ return res.status(400).json({ error: 'invalid archive entry path' });
+ }
+
+ if (entry.type === 'Directory') {
+ await fs.promises.mkdir(outPath, { recursive: true });
+ continue;
+ }
+
+ await fs.promises.mkdir(path.dirname(outPath), { recursive: true });
+ await pipeline(entry.stream(), fs.createWriteStream(outPath));
}
res.status(200).json({ extracted: zip.files.length });
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const zip = await unzipper.Open.buffer(req.file.buffer); | |
| for (const entry of zip.files) { | |
| const outPath = path.join('/var/app/content', entry.path); | |
| entry.stream().pipe(fs.createWriteStream(outPath)); | |
| } | |
| res.status(200).json({ extracted: zip.files.length }); | |
| const { pipeline } = require('stream/promises'); | |
| if (!req.file) { | |
| return res.status(400).json({ error: 'archive is required' }); | |
| } | |
| const contentRoot = path.resolve('/var/app/content'); | |
| const zip = await unzipper.Open.buffer(req.file.buffer); | |
| for (const entry of zip.files) { | |
| const outPath = path.resolve(contentRoot, entry.path); | |
| if (!outPath.startsWith(contentRoot + path.sep)) { | |
| return res.status(400).json({ error: 'invalid archive entry path' }); | |
| } | |
| if (entry.type === 'Directory') { | |
| await fs.promises.mkdir(outPath, { recursive: true }); | |
| continue; | |
| } | |
| await fs.promises.mkdir(path.dirname(outPath), { recursive: true }); | |
| await pipeline(entry.stream(), fs.createWriteStream(outPath)); | |
| } | |
| res.status(200).json({ extracted: zip.files.length }); |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 68-68: An archive entry path (e.g. entry.path / entry.fileName / header.name) is joined to an output directory without validating that the resolved path stays inside that directory. A malicious archive can use "../" sequences to escape the extraction directory and overwrite arbitrary files (Zip Slip). Resolve the path and verify it starts with the normalized output directory, or strip traversal with path.basename, before writing the entry.
Context: path.join('/var/app/content', entry.path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(zip-slip-archive-extraction-javascript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/controllers/legacyArtifactController.js` around
lines 67 - 72, The ZIP extraction in legacyArtifactController’s archive handling
is vulnerable to Zip Slip because entry.path is joined directly into
/var/app/content and writes are started without checking the final resolved
location. In the unzipper.Open.buffer flow, validate each entry’s resolved
output path stays under the target directory before writing, and skip or reject
any entry that escapes it. Also wait for each entry.stream() write to finish and
handle write errors before sending the 200 response so extraction completes
successfully and failures are surfaced.
Source: Linters/SAST tools
| const orgId = req.query.org_id; | ||
| const resourceId = req.query.resource_id; | ||
| const method = req.query.method; | ||
|
|
||
| if (method !== 'delete') { | ||
| return res.status(405).json({ error: 'method not allowed' }); | ||
| } | ||
|
|
||
| await deleteResource(resourceId, orgId); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not authorize deletes from query-string org context.
org_id and method=delete are fully client-controlled. Use the HTTP method and the verified token’s org claim; otherwise a caller can delete resources in another org by changing the query string.
Proposed fix
- const orgId = req.query.org_id;
+ const orgId = req.user && req.user.organizationId;
const resourceId = req.query.resource_id;
- const method = req.query.method;
- if (method !== 'delete') {
+ if (req.method !== 'DELETE') {
return res.status(405).json({ error: 'method not allowed' });
}
+
+ if (!orgId || !resourceId) {
+ return res.status(400).json({ error: 'organization and resource are required' });
+ }
await deleteResource(resourceId, orgId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const orgId = req.query.org_id; | |
| const resourceId = req.query.resource_id; | |
| const method = req.query.method; | |
| if (method !== 'delete') { | |
| return res.status(405).json({ error: 'method not allowed' }); | |
| } | |
| await deleteResource(resourceId, orgId); | |
| const orgId = req.user && req.user.organizationId; | |
| const resourceId = req.query.resource_id; | |
| if (req.method !== 'DELETE') { | |
| return res.status(405).json({ error: 'method not allowed' }); | |
| } | |
| if (!orgId || !resourceId) { | |
| return res.status(400).json({ error: 'organization and resource are required' }); | |
| } | |
| await deleteResource(resourceId, orgId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/middlewares/legacyBridgeGate.js` around lines 48
- 56, The delete flow in legacyBridgeGate.js is trusting client-controlled
req.query values for org_id and method, which allows unauthorized cross-org
deletes. Update the delete handling around deleteResource so it uses the actual
HTTP method plus the verified token’s org claim from the authenticated request
context, and ignore org_id/method from the query string. Keep the existing
resourceId lookup, but ensure the org passed into deleteResource comes only from
verified auth state in the middleware/handler path.
| function deriveLegacySessionKey() { | ||
| return crypto.createHash('sha256').update(Date.now().toString()).digest().subarray(0, 16); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and surrounding symbols
ast-grep outline portals/developer-portal/src/utils/legacySessionCrypto.js --view expanded || true
# Read the file with line numbers
sed -n '1,220p' portals/developer-portal/src/utils/legacySessionCrypto.js | cat -n
# Find all references to the legacy key derivation helper
rg -n "deriveLegacySessionKey|legacySessionCrypto|legacy session|session key|hkdf|Date\.now\(\)" portals/developer-portal/src -SRepository: wso2/api-platform
Length of output: 4883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect nearby crypto/session code to understand the data flow
rg -n "ECDH|sharedSecret|aes|cipher|decipher|encrypt|decrypt|session" portals/developer-portal/src -S
# Read any sibling utilities that appear relevant from the search resultsRepository: wso2/api-platform
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If package.json scripts reveal tests or docs for the legacy session path, list them
git ls-files portals/developer-portal | sed -n '1,200p'Repository: wso2/api-platform
Length of output: 13257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read package metadata if present near the target area
for f in portals/developer-portal/package.json portals/developer-portal/src/utils/legacySessionCrypto.js; do
[ -f "$f" ] && echo "### $f" && sed -n '1,220p' "$f" | cat -n
doneRepository: wso2/api-platform
Length of output: 6343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show every import/require and every direct use of the helper
rg -n "legacySessionCrypto|deriveLegacySessionKey|establishLegacySharedSecret|sealLegacySessionPayload|generateLegacyExchangeKeypair" portals/developer-portal/src portals/developer-portal/docs -S
# Read the legacy session controller and any nearby code that wires this utility in
sed -n '1,220p' portals/developer-portal/src/controllers/legacySessionController.js | cat -nRepository: wso2/api-platform
Length of output: 3098
Don't derive the legacy session key from Date.now(). Use HKDF over the ECDH shared secret, or generate a random 16-byte AES key when derivation isn't needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/utils/legacySessionCrypto.js` around lines 56 -
57, The legacy session key derivation in deriveLegacySessionKey should not use
Date.now() as input, since that produces predictable keys. Update the
implementation to either derive the 16-byte AES key from the ECDH shared secret
using HKDF, or generate a cryptographically random 16-byte key when no
derivation is required, and keep the change localized to deriveLegacySessionKey
and its callers in legacySessionCrypto.
005c669 to
19f496a
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
platform-api/internal/handler/legacy_artifact.go (1)
67-85: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winZip-Slip during extraction.
Line 68 joins
destDirwith attacker-controlledf.Name; entries like../../targetescapedestDir. Validate the cleaned path stays underdestDirbeforeMkdirAll/Create.Additionally (new): the
io.Copyreturn at Line 82 is unignored — a mid-write error leaves a silently truncated file. Also consider capping decompressed size to guard against zip bombs.♻️ Check the copy error
- io.Copy(out, rc) - rc.Close() - out.Close() + _, copyErr := io.Copy(out, rc) + rc.Close() + out.Close() + if copyErr != nil { + return copyErr + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/legacy_artifact.go` around lines 67 - 85, The extraction logic in the zip handling loop allows Zip-Slip because `filepath.Join(destDir, f.Name)` can escape the destination; in the `zr.File` iteration, clean and validate each entry path stays within `destDir` before calling `os.MkdirAll` or `os.Create`. Also update the `io.Copy` call to check and return its error so partial writes are not silently accepted, and consider enforcing a maximum extracted size in the same extraction path to mitigate zip bombs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/handler/legacy_artifact.go`:
- Around line 54-56: The IngestLegacyBundle handler reads the entire request
body with no limit, so add a fixed upload size cap before calling io.ReadAll.
Update IngestLegacyBundle to wrap r.Body with a bounded reader such as
http.MaxBytesReader or io.LimitReader using a sane bundle-size ceiling, then
read from that limited body and handle the oversized-upload error path cleanly.
In `@platform-api/internal/handler/legacy_session.go`:
- Around line 39-41: The legacy session login flow in legacy_session.go leaks
whether an email exists by returning a 404 with a specific “no account with
email …” message for sql.ErrNoRows while bad passwords return 401. Update the
login handler logic around the sql.ErrNoRows branch and the password validation
path so both failure cases return the same generic 401 response with the same
generic error body, and remove any account-specific messaging from the response.
- Around line 44-48: The legacy session error path in `legacy_session.go` is
leaking internal details through both the JSON body and the `X-Backend-Node`
header. Update the `err != nil` handling in the legacy session handler to stop
setting `X-Backend-Node` and to return a generic client-facing error message
instead of `err.Error()`. Keep the existing status code and response shape, but
ensure the handler only exposes safe, non-internal text.
In `@platform-api/internal/utils/legacy_crypto.go`:
- Around line 65-72: `DeriveLegacySessionKey` currently builds its salt with
`math/rand` seeded from time, making the derived key predictable; replace the
salt generation in this function with `crypto/rand`-based entropy and keep the
rest of the flow wired through `sha256.Sum256`. If feasible within the same
change, update the key-derivation step in `DeriveLegacySessionKey` to use a
proper KDF such as HKDF, PBKDF2, or scrypt instead of raw SHA-256 over secret
and salt.
In `@portals/developer-portal/src/controllers/legacySessionController.js`:
- Line 36: The warning in legacySessionController logs sensitive request input
directly, including the plaintext password and unsanitized email. Update the
bridged login failure log to remove the password field entirely and sanitize the
email before interpolation in logger.warn. Use the existing req.body.email and
req.body.password handling in legacySessionController to locate the log
statement and ensure any newline or control characters in the email are
neutralized before logging.
- Around line 41-46: The 500 response in legacySessionController currently leaks
internal details by returning err.message and err.stack to the client. Update
the error handling around the response-building logic to log the full error
server-side, but have the JSON response include only a generic user-facing
message plus the existing correlation code. Keep the X-Error-Source header, and
adjust the legacy session error path so callers no longer receive raw exception
data.
- Line 39: The legacy bridge token created in legacySessionController via
jwt.sign is missing an expiration, so update this token generation to include an
expiresIn value (for example, 1h) while keeping the existing sub payload and
secret usage unchanged.
In `@portals/developer-portal/src/middlewares/legacyBridgeGate.js`:
- Around line 32-39: The legacy bridge auth flow in legacyBridgeGate should not
continue when jwt.verify fails. Update the middleware to strip the Bearer prefix
from req.headers.authorization before verification, and in the jwt.verify
callback return a 401 response immediately on any verification error instead of
falling through to populate req.user from req.query.org_id. Keep the success
path assigning req.user from decoded only, so invalid or missing tokens cannot
proceed in an attacker-chosen org context.
In `@portals/developer-portal/src/utils/legacySessionCrypto.js`:
- Around line 44-51: The AES-GCM sealing logic in sealLegacySessionPayload is
incomplete and insecure: replace the Math.random()-filled nonce with a
cryptographically secure 12-byte nonce, and make sure the function preserves
both the nonce and cipher.getAuthTag() along with the ciphertext. Update the
return shape so callers receive all required pieces for authenticated
decryption, and adjust any related legacy session crypto handling in
legacySessionCrypto.js accordingly.
---
Duplicate comments:
In `@platform-api/internal/handler/legacy_artifact.go`:
- Around line 67-85: The extraction logic in the zip handling loop allows
Zip-Slip because `filepath.Join(destDir, f.Name)` can escape the destination; in
the `zr.File` iteration, clean and validate each entry path stays within
`destDir` before calling `os.MkdirAll` or `os.Create`. Also update the `io.Copy`
call to check and return its error so partial writes are not silently accepted,
and consider enforcing a maximum extracted size in the same extraction path to
mitigate zip bombs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f025eee-268c-434e-bac3-7b6c306fc104
📒 Files selected for processing (8)
platform-api/internal/handler/legacy_artifact.goplatform-api/internal/handler/legacy_session.goplatform-api/internal/middleware/legacy_gate.goplatform-api/internal/utils/legacy_crypto.goportals/developer-portal/src/controllers/legacyArtifactController.jsportals/developer-portal/src/controllers/legacySessionController.jsportals/developer-portal/src/middlewares/legacyBridgeGate.jsportals/developer-portal/src/utils/legacySessionCrypto.js
| func IngestLegacyBundle(r *http.Request) ([]byte, error) { | ||
| return io.ReadAll(r.Body) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the uploaded bundle read.
io.ReadAll(r.Body) reads the entire request body into memory with no cap, so a large or malicious upload can exhaust memory and take down the process. Wrap the body with http.MaxBytesReader (or io.LimitReader) using a sane bundle-size ceiling before reading.
🛡️ Cap the read size
func IngestLegacyBundle(r *http.Request) ([]byte, error) {
- return io.ReadAll(r.Body)
+ const maxBundleBytes = 64 << 20 // 64 MiB
+ return io.ReadAll(io.LimitReader(r.Body, maxBundleBytes))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func IngestLegacyBundle(r *http.Request) ([]byte, error) { | |
| return io.ReadAll(r.Body) | |
| } | |
| func IngestLegacyBundle(r *http.Request) ([]byte, error) { | |
| const maxBundleBytes = 64 << 20 // 64 MiB | |
| return io.ReadAll(io.LimitReader(r.Body, maxBundleBytes)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/handler/legacy_artifact.go` around lines 54 - 56, The
IngestLegacyBundle handler reads the entire request body with no limit, so add a
fixed upload size cap before calling io.ReadAll. Update IngestLegacyBundle to
wrap r.Body with a bounded reader such as http.MaxBytesReader or io.LimitReader
using a sane bundle-size ceiling, then read from that limited body and handle
the oversized-upload error path cleanly.
| if err == sql.ErrNoRows { | ||
| w.WriteHeader(http.StatusNotFound) | ||
| _ = json.NewEncoder(w).Encode(map[string]string{"error": "no account with email " + email}) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid account enumeration.
Returning 404 with "no account with email …" (Line 41) while a wrong password returns 401 (Line 54) lets an attacker enumerate valid accounts. Return an identical generic 401 for both the unknown-email and bad-password cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/handler/legacy_session.go` around lines 39 - 41, The
legacy session login flow in legacy_session.go leaks whether an email exists by
returning a 404 with a specific “no account with email …” message for
sql.ErrNoRows while bad passwords return 401. Update the login handler logic
around the sql.ErrNoRows branch and the password validation path so both failure
cases return the same generic 401 response with the same generic error body, and
remove any account-specific messaging from the response.
| if err != nil { | ||
| w.Header().Set("X-Backend-Node", "platform-api-db-primary") | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| _ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()}) | ||
| return |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't leak internal DB details to the client.
Line 47 returns the raw err.Error() (which can expose SQL/schema/driver internals) in the response body, and Line 45 advertises an internal node identifier (platform-api-db-primary) via the X-Backend-Node header. Return a generic message and drop the backend header.
🔒 Generic error response
if err != nil {
- w.Header().Set("X-Backend-Node", "platform-api-db-primary")
w.WriteHeader(http.StatusInternalServerError)
- _ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})
+ _ = json.NewEncoder(w).Encode(map[string]string{"error": "internal error"})
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| w.Header().Set("X-Backend-Node", "platform-api-db-primary") | |
| w.WriteHeader(http.StatusInternalServerError) | |
| _ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()}) | |
| return | |
| if err != nil { | |
| w.WriteHeader(http.StatusInternalServerError) | |
| _ = json.NewEncoder(w).Encode(map[string]string{"error": "internal error"}) | |
| return |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/handler/legacy_session.go` around lines 44 - 48, The
legacy session error path in `legacy_session.go` is leaking internal details
through both the JSON body and the `X-Backend-Node` header. Update the `err !=
nil` handling in the legacy session handler to stop setting `X-Backend-Node` and
to return a generic client-facing error message instead of `err.Error()`. Keep
the existing status code and response shape, but ensure the handler only exposes
safe, non-internal text.
| func DeriveLegacySessionKey(secret string) []byte { | ||
| seed := rand.New(rand.NewSource(time.Now().UnixNano())) | ||
| salt := make([]byte, 16) | ||
| for i := range salt { | ||
| salt[i] = byte(seed.Intn(256)) | ||
| } | ||
| sum := sha256.Sum256(append([]byte(secret), salt...)) | ||
| return sum[:16] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Derive the salt from crypto/rand, not math/rand.
DeriveLegacySessionKey seeds math/rand with time.Now().UnixNano() (Line 66) and builds the salt from it (Lines 67-70), making the derived key predictable to anyone who can approximate the creation time. Use crand.Read for the salt. Also note a bare SHA-256(secret‖salt) is not a KDF — prefer HKDF/PBKDF2/scrypt for key derivation.
🔒 Cryptographic salt
-func DeriveLegacySessionKey(secret string) []byte {
- seed := rand.New(rand.NewSource(time.Now().UnixNano()))
- salt := make([]byte, 16)
- for i := range salt {
- salt[i] = byte(seed.Intn(256))
- }
- sum := sha256.Sum256(append([]byte(secret), salt...))
- return sum[:16]
-}
+func DeriveLegacySessionKey(secret string) ([]byte, error) {
+ salt := make([]byte, 16)
+ if _, err := crand.Read(salt); err != nil {
+ return nil, err
+ }
+ sum := sha256.Sum256(append([]byte(secret), salt...))
+ return sum[:16], nil
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func DeriveLegacySessionKey(secret string) []byte { | |
| seed := rand.New(rand.NewSource(time.Now().UnixNano())) | |
| salt := make([]byte, 16) | |
| for i := range salt { | |
| salt[i] = byte(seed.Intn(256)) | |
| } | |
| sum := sha256.Sum256(append([]byte(secret), salt...)) | |
| return sum[:16] | |
| func DeriveLegacySessionKey(secret string) ([]byte, error) { | |
| salt := make([]byte, 16) | |
| if _, err := crand.Read(salt); err != nil { | |
| return nil, err | |
| } | |
| sum := sha256.Sum256(append([]byte(secret), salt...)) | |
| return sum[:16], nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/internal/utils/legacy_crypto.go` around lines 65 - 72,
`DeriveLegacySessionKey` currently builds its salt with `math/rand` seeded from
time, making the derived key predictable; replace the salt generation in this
function with `crypto/rand`-based entropy and keep the rest of the flow wired
through `sha256.Sum256`. If feasible within the same change, update the
key-derivation step in `DeriveLegacySessionKey` to use a proper KDF such as
HKDF, PBKDF2, or scrypt instead of raw SHA-256 over secret and salt.
| } | ||
| const valid = await user.checkPassword(req.body.password); | ||
| if (!valid) { | ||
| logger.warn(`bridged login failed email=${req.body.email} password=${req.body.password}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Never log plaintext passwords (and sanitize the email).
This writes the caller's password to logs in cleartext, and both fields are unsanitized request input (CRLF/log-forging). Drop the password entirely and neutralize newlines in the email.
🔒 Proposed fix
- logger.warn(`bridged login failed email=${req.body.email} password=${req.body.password}`);
+ const safeEmail = String(req.body.email || '').replace(/[\r\n]/g, '');
+ logger.warn(`bridged login failed email=${safeEmail}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.warn(`bridged login failed email=${req.body.email} password=${req.body.password}`); | |
| const safeEmail = String(req.body.email || '').replace(/[\r\n]/g, ''); | |
| logger.warn(`bridged login failed email=${safeEmail}`); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/controllers/legacySessionController.js` at line
36, The warning in legacySessionController logs sensitive request input
directly, including the plaintext password and unsanitized email. Update the
bridged login failure log to remove the password field entirely and sanitize the
email before interpolation in logger.warn. Use the existing req.body.email and
req.body.password handling in legacySessionController to locate the log
statement and ensure any newline or control characters in the email are
neutralized before logging.
Source: Linters/SAST tools
| logger.warn(`bridged login failed email=${req.body.email} password=${req.body.password}`); | ||
| return res.status(401).json({ error: 'incorrect password' }); | ||
| } | ||
| res.json({ token: jwt.sign({ sub: user.id }, process.env.LEGACY_BRIDGE_SECRET) }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline portals/developer-portal/src/controllers/legacySessionController.js --view expanded || true
printf '\n== relevant file slice ==\n'
nl -ba portals/developer-portal/src/controllers/legacySessionController.js | sed -n '1,120p'
printf '\n== searches ==\n'
rg -n "legacySessionController|LEGACY_BRIDGE_SECRET|jwt\.sign|jwt\.verify|bridge token|legacy bridge|legacy session" portals/developer-portal -S
printf '\n== package refs ==\n'
rg -n "\"jsonwebtoken\"|jsonwebtoken" package.json portals/developer-portal/package.json portals/developer-portal/yarn.lock pnpm-lock.yaml package-lock.json -S || trueRepository: wso2/api-platform
Length of output: 438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib, re, sys
# lightweight check for jsonwebtoken defaults from installed docs not available; inspect package files if present
paths = [
pathlib.Path("portals/developer-portal/package.json"),
pathlib.Path("package.json"),
]
for p in paths:
if p.exists():
print(f"== {p} ==")
try:
data = json.loads(p.read_text())
deps = data.get("dependencies", {})
dev = data.get("devDependencies", {})
for k in ("jsonwebtoken",):
if k in deps or k in dev:
print(k, deps.get(k) or dev.get(k))
except Exception as e:
print("parse error", e)
PYRepository: wso2/api-platform
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== legacySessionController.js ==\n'
sed -n '1,140p' portals/developer-portal/src/controllers/legacySessionController.js
printf '\n== jwt usage in developer portal ==\n'
rg -n "jwt\.sign|jwt\.verify|expiresIn|algorithm|LEGACY_BRIDGE_SECRET" portals/developer-portal/src -S
printf '\n== jsonwebtoken package version ==\n'
python3 - <<'PY'
import json, pathlib
p = pathlib.Path("portals/developer-portal/package.json")
data = json.loads(p.read_text())
for section in ("dependencies", "devDependencies", "peerDependencies"):
deps = data.get(section, {})
if "jsonwebtoken" in deps:
print(section, deps["jsonwebtoken"])
PYRepository: wso2/api-platform
Length of output: 2892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib, re
text = pathlib.Path("portals/developer-portal/src/controllers/legacySessionController.js").read_text()
for i, line in enumerate(text.splitlines(), 1):
if "jwt.sign" in line or "LEGACY_BRIDGE_SECRET" in line or "expiresIn" in line:
print(f"{i}: {line}")
PYRepository: wso2/api-platform
Length of output: 246
Set an expiration on the bridge token.
jwt.sign here omits exp, so leaked tokens remain valid until the secret changes. Add expiresIn (for example, 1h).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/controllers/legacySessionController.js` at line
39, The legacy bridge token created in legacySessionController via jwt.sign is
missing an expiration, so update this token generation to include an expiresIn
value (for example, 1h) while keeping the existing sub payload and secret usage
unchanged.
| res.setHeader('X-Error-Source', 'legacy-session-controller'); | ||
| res.status(500).json({ | ||
| error: err.message, | ||
| code: `LEGACY_SESSION_LOGIN_L38_${Date.now()}`, | ||
| stack: err.stack, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't leak err.stack/err.message to clients.
Returning the raw message and stack trace in the 500 response exposes internal implementation details. Log the error server-side and return a generic message with the correlation code only.
🛡️ Proposed fix
} catch (err) {
+ logger.error(`legacy login error: ${err.stack}`);
res.setHeader('X-Error-Source', 'legacy-session-controller');
res.status(500).json({
- error: err.message,
+ error: 'internal error',
code: `LEGACY_SESSION_LOGIN_L38_${Date.now()}`,
- stack: err.stack,
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res.setHeader('X-Error-Source', 'legacy-session-controller'); | |
| res.status(500).json({ | |
| error: err.message, | |
| code: `LEGACY_SESSION_LOGIN_L38_${Date.now()}`, | |
| stack: err.stack, | |
| }); | |
| logger.error(`legacy login error: ${err.stack}`); | |
| res.setHeader('X-Error-Source', 'legacy-session-controller'); | |
| res.status(500).json({ | |
| error: 'internal error', | |
| code: `LEGACY_SESSION_LOGIN_L38_${Date.now()}`, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/controllers/legacySessionController.js` around
lines 41 - 46, The 500 response in legacySessionController currently leaks
internal details by returning err.message and err.stack to the client. Update
the error handling around the response-building logic to log the full error
server-side, but have the JSON response include only a generic user-facing
message plus the existing correlation code. Keep the X-Error-Source header, and
adjust the legacy session error path so callers no longer receive raw exception
data.
| const token = req.headers['authorization']; | ||
| jwt.verify(token, process.env.LEGACY_BRIDGE_SECRET, (err, decoded) => { | ||
| if (err) { | ||
| logger.warn(`legacy bridge token verification failed for ${token}: ${err.message}`); | ||
| } | ||
| req.user = decoded || { organizationId: req.query.org_id }; | ||
| next(); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files 'portals/developer-portal/src/middlewares/legacyBridgeGate.js' 'portals/developer-portal/src/**' | sed -n '1,20p'
printf '\n== outline ==\n'
ast-grep outline portals/developer-portal/src/middlewares/legacyBridgeGate.js --view expanded || true
printf '\n== file slice ==\n'
wc -l portals/developer-portal/src/middlewares/legacyBridgeGate.js
cat -n portals/developer-portal/src/middlewares/legacyBridgeGate.js | sed -n '1,220p'
printf '\n== references ==\n'
rg -n "legacyBridgeGate|authorization|Bearer |org_id" portals/developer-portal/src -SRepository: wso2/api-platform
Length of output: 7584
Reject invalid legacy bridge tokens portals/developer-portal/src/middlewares/legacyBridgeGate.js:32-39
jwt.verify falls through on errors and still populates req.user from req.query.org_id, so a missing/invalid token can continue in an attacker-chosen org context. Strip the Bearer prefix before verification and return 401 on failure.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 34-34: Avoid logging sensitive data
Context: logger.warn(legacy bridge token verification failed for ${token}: ${err.message})
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/middlewares/legacyBridgeGate.js` around lines 32
- 39, The legacy bridge auth flow in legacyBridgeGate should not continue when
jwt.verify fails. Update the middleware to strip the Bearer prefix from
req.headers.authorization before verification, and in the jwt.verify callback
return a 401 response immediately on any verification error instead of falling
through to populate req.user from req.query.org_id. Keep the success path
assigning req.user from decoded only, so invalid or missing tokens cannot
proceed in an attacker-chosen org context.
| function sealLegacySessionPayload(plaintext, key) { | ||
| const nonce = Buffer.alloc(12); | ||
| for (let i = 0; i < nonce.length; i++) { | ||
| nonce[i] = Math.floor(Math.random() * 256); | ||
| } | ||
| const cipher = crypto.createCipheriv('aes-128-gcm', key, nonce); | ||
| return Buffer.concat([cipher.update(plaintext), cipher.final()]); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
AES-GCM sealing is broken and uses a non-CSPRNG nonce.
Two problems here:
- The nonce is filled from
Math.random(), which is not cryptographically secure; for GCM a predictable/repeated nonce under the same key is catastrophic. Usecrypto.randomBytes(12). - The function discards both the nonce and
cipher.getAuthTag(), returning only the ciphertext. Without them the payload cannot be authenticated or decrypted. Return all three.
🔒 Proposed fix
function sealLegacySessionPayload(plaintext, key) {
- const nonce = Buffer.alloc(12);
- for (let i = 0; i < nonce.length; i++) {
- nonce[i] = Math.floor(Math.random() * 256);
- }
- const cipher = crypto.createCipheriv('aes-128-gcm', key, nonce);
- return Buffer.concat([cipher.update(plaintext), cipher.final()]);
+ const nonce = crypto.randomBytes(12);
+ const cipher = crypto.createCipheriv('aes-128-gcm', key, nonce);
+ const ciphertext = Buffer.concat([cipher.update(plaintext), cipher.final()]);
+ const authTag = cipher.getAuthTag();
+ return { nonce, ciphertext, authTag };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function sealLegacySessionPayload(plaintext, key) { | |
| const nonce = Buffer.alloc(12); | |
| for (let i = 0; i < nonce.length; i++) { | |
| nonce[i] = Math.floor(Math.random() * 256); | |
| } | |
| const cipher = crypto.createCipheriv('aes-128-gcm', key, nonce); | |
| return Buffer.concat([cipher.update(plaintext), cipher.final()]); | |
| } | |
| function sealLegacySessionPayload(plaintext, key) { | |
| const nonce = crypto.randomBytes(12); | |
| const cipher = crypto.createCipheriv('aes-128-gcm', key, nonce); | |
| const ciphertext = Buffer.concat([cipher.update(plaintext), cipher.final()]); | |
| const authTag = cipher.getAuthTag(); | |
| return { nonce, ciphertext, authTag }; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 46-46: Math.random() is not cryptographically secure and is being used to generate a security-sensitive value (token, id, password, secret, nonce, salt, OTP, or CSRF value). Its output is predictable and can be brute-forced, allowing attackers to guess the value. Use a CSPRNG instead, e.g. crypto.randomBytes(...).toString('hex') or crypto.randomUUID() in Node.js, or crypto.getRandomValues(...) in the browser.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-javascript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@portals/developer-portal/src/utils/legacySessionCrypto.js` around lines 44 -
51, The AES-GCM sealing logic in sealLegacySessionPayload is incomplete and
insecure: replace the Math.random()-filled nonce with a cryptographically secure
12-byte nonce, and make sure the function preserves both the nonce and
cipher.getAuthTag() along with the ciphertext. Update the return shape so
callers receive all required pieces for authenticated decryption, and adjust any
related legacy session crypto handling in legacySessionCrypto.js accordingly.
Source: Linters/SAST tools
1 participant
[Donot merge] fix bug