Repository navigation
fix(redshift-alpha): backslashes in user passwords and comments are not preserved - #39306
Conversation
…ot preserved Redshift reads backslash escapes in plain string literals. A backslash in a user password or a table or column comment could be stored altered or make the statement fail. - Escape backslashes in `quoteLiteral` before doubling single quotes, so the value reaches Redshift as written. - Apply this to the CREATE USER and ALTER USER password and to the COMMENT ON TABLE and COMMENT ON COLUMN text. - Leave stored passwords and comments unchanged on upgrade. The next time a comment is sent, an escape such as `\n` in it is stored literally. - Add an acknowledgeable synthesis warning when `User` `excludeCharacters` doesn't include `'`, `"`, `/` or `@`, which the [CREATE USER](https://docs.aws.amazon.com/redshift/latest/dg/r_CREATE_USER.html) documentation disallows in clear-text passwords. - Leave `\` out of that warning, although the documentation lists it, because Redshift accepted a password containing an escaped `\`. Validation: - Build, lint and 291 unit tests pass. - Regenerated the six integ snapshots that embed the query provider; their templates change only in its asset hash. - On a development account, a build with this change stored backslashes in table and column comments exactly on create and update, and created a user who could log in with a backslash password. - ALTER USER was not run live.
|
PRs without a linked issue will receive lower priority for review and merging. Please update the description to follow the PR template and include a line like |
|
|
||||||||||||||
|
|
||||||||||||||
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit Note: part of this change couldn't be included in this automated review, so it may be incomplete. This PR fixes backslash handling in the experimental The change is small, additive, and well-formed. The escaping order is correct (quote-doubling inserts no backslash, so no re-escaping), the new warning is non-blocking and correctly skipped for 🔴 0 blocking · 🟡 1 recommended · ⚪ 0 optional Files with findings (1)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
![maintainer-for-aws[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| */ | ||
| export function quoteLiteral(literal: string): string { | ||
| return `'${literal.replace(/'/g, "''")}'`; | ||
| return `'${literal.replace(/\\/g, '\\\\').replace(/'/g, "''")}'`; |
There was a problem hiding this comment.
🟡 Recommended — The quoteLiteral fix escapes backslashes before doubling single quotes, changing how the Redshift query-provider Custom Resource formats password and COMMENT SQL. That correctness is only observable at deploy time, because the SQL executes inside the provider Lambda, so no synth snapshot can catch it — the six regenerated integ snapshots are only asset-hash churn. The existing integ stacks exercise no backslash: the exclude-characters stack passes an excludeCharacters set that already contains all five characters, and the database stack uses only plain comments. Combined with the author's note that the ALTER USER password path was not run live, the end-to-end round-trip this PR fixes has no real-deploy verification (INTEGRATION_TESTS.md).
Suggested change: Add a user with a backslash-bearing password and a table/column COMMENT containing a backslash to one of the existing integ stacks, with a deploy-time assertion (an awsApiCall with an ExpectedResult, e.g. reading the stored comment back via the Redshift Data API) confirming the backslash survives the round-trip. If a live Redshift deploy is genuinely impractical, apply the pr-linter/exempt-integ-test label with a note that the handler unit tests stand in for it.
There was a problem hiding this comment.
Unit tests cover this: they assert the exact SQL for passwords and for table and column comments that contain backslashes.
aws-cdk-automation
added
the
pr/needs-further-review
ozelalisen
added
the
pr-linter/exempt-integ-test
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status
This pull request spent 48 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Requeued — the merge queue status continues in this comment ↓. |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status
This pull request spent 23 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
|
Comments on closed issues and PRs are hard for our team to see. |
This branch was successfully deployed
3 participants
Reason for this change
Redshift reads backslash escapes in plain string literals. A backslash in a user password or a table or column comment could be stored altered or make the statement fail.
Description of changes
quoteLiteralbefore doubling single quotes, so the value reaches Redshift as written.\nin it is stored literally.UserexcludeCharactersdoesn't include',",/or@, which the CREATE USER documentation disallows in clear-text passwords.\out of that warning, although the documentation lists it, because Redshift accepted a password containing an escaped\.Describe any new or updated permissions being added
No new permissions.
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license