Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
pru55e11
left a comment
pru55e11
left a comment
There was a problem hiding this comment.
Issue found by cursor:
One issue with the test: the assertion not.toEqual(expect.stringContaining('secret')) will fail when run in generated projects. After hiding, the output JSON still contains the key name client_secret, which includes the substring secret. So stringContaining('secret') matches and the not assertion fails.
This passes CI here because the template test isn't executed in this repo, but developers will hit it when they run tests in their generated apps.
Suggested fix:
expect(utils.stringParameters(params)).not.toEqual(expect.stringContaining('"secret"'))
(Checking for the quoted value "secret" instead of the bare substring secret avoids matching the key name client_secret.)
There was a problem hiding this comment.
Pull request overview
This PR updates the common action-template logging utility to avoid leaking IMS S2S credentials (specifically client_secret) when action input params are stringified for debug logging.
Changes:
- Redacts
params.__ims_oauth_s2s.client_secretinstringParameters. - Adds a unit test asserting IMS credential redaction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/common-templates/utils.js | Redacts client_secret when serializing params for logs. |
| lib/common-templates/utils.test.js | Adds coverage for IMS credential redaction in stringParameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function stringParameters (params) { | ||
| // hide credentials from the include-ims-credentials annotation | ||
| let imsCredentials = params.__ims_oauth_s2s || {} | ||
| if (imsCredentials.client_secret) { | ||
| imsCredentials = { ...imsCredentials, client_secret: '<hidden>' } | ||
| } | ||
| // hide authorization token without overriding params | ||
| let headers = params.__ow_headers || {} | ||
| if (headers.authorization) { | ||
| headers = { ...headers, authorization: '<hidden>' } | ||
| } | ||
| return JSON.stringify({ ...params, __ow_headers: headers }) | ||
|
|
||
| return JSON.stringify({ ...params, __ow_headers: headers, __ims_oauth_s2s: imsCredentials }) |
There was a problem hiding this comment.
stringParameters now always injects __ims_oauth_s2s: {} into the returned JSON when the input params has no __ims_oauth_s2s. This changes the previous output shape and will break the existing "no auth header" test expectation (and any callers relying on the logged JSON matching the original params). Consider only adding __ims_oauth_s2s to the serialized object when it exists on the input params (or when it’s non-empty).
| a: 1, b: 2, __ims_oauth_s2s: { client_id: 'fake-client-id', client_secret: 'secret', org_id: 'fake@AdobeOrg' } | ||
| } | ||
| expect(utils.stringParameters(params)).toEqual(expect.stringContaining('"client_secret":"<hidden>"')) | ||
| expect(utils.stringParameters(params)).not.toEqual(expect.stringContaining('secret')) |
There was a problem hiding this comment.
The new test’s not.toEqual(expect.stringContaining('secret')) assertion will fail because the output necessarily contains the substring secret in the key name client_secret (even when the value is properly hidden). Adjust the assertion to specifically check that the value isn't leaked (e.g., ensure it does not contain "client_secret":"secret", or parse the JSON and assert client_secret === '<hidden>').
| expect(utils.stringParameters(params)).not.toEqual(expect.stringContaining('secret')) | |
| expect(utils.stringParameters(params)).not.toEqual(expect.stringContaining('"client_secret":"secret"')) |
There was a problem hiding this comment.
Copilot and Cursor agree on the issue, but have different suggestions for how to fix it.
4 participants
Description
relates to adobe/aio-lib-runtime#224
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: