fix(security): audit fixes for secret redaction, error sanitization and HTTP hardening#14
Draft
JulienJBO wants to merge 6 commits into
Draft
fix(security): audit fixes for secret redaction, error sanitization and HTTP hardening#14JulienJBO wants to merge 6 commits into
JulienJBO wants to merge 6 commits into
Conversation
normalizeError() previously wrapped non-Error values via `new Error(msg, {
cause: error })`. Node ≥ 16 prints the cause chain verbatim through
util.inspect / Error.toString, which can resurface unsanitized payloads
(1Password tokens, Connect URLs with credentials) downstream.
The wrapper now keeps only the redacted message. The top-level fatal
handler in index.ts is routed through errorMessage() for the same reason.
redactExactValue used a /g regex without the `i` flag, so a configured secretRedactionValues entry like `MyAPIKey` would survive in `op` output that happens to lowercase it (URL hosts, hex tokens, error messages). Aligns with the /gi pattern already used by redactAuthText for OP_SESSION / OP_SERVICE_ACCOUNT_TOKEN.
sanitizeAuditValue walked objects via Object.entries, which skips Symbol keys and non-enumerable properties. Some SDKs (AWS, Octokit, undici errors) hide credentials on non-enumerable property descriptors; those would slip through audit redaction if such an object reached metadata or error payloads. Rewritten to traverse Reflect.ownKeys with explicit descriptor reads. The function is now exported for direct testing.
Two related issues in parseSecretReference / equalsIdentifier: * decodeURIComponent on a malformed escape (e.g. `%E0` without a continuation byte) raised a raw URIError that escaped the wrapper. Now caught and re-thrown as a typed Invalid secret reference error, consistent with the rest of the parser. * Field title matching compared lowercase trims directly, so a stored NFC label (`café` = U+00E9) failed to match an NFD query (`café` = e + U+0301). macOS keyboards routinely emit NFD for accented Latin characters; both forms now normalize to NFC before comparison.
The DNS rebinding protection only relied on the Origin header, which non-browser clients can omit entirely (assertOriginAllowed returns true when Origin is absent). A local process reaching 127.0.0.1 via a rebound DNS name would therefore bypass the check. Adds an assertHostAllowed step that whitelists the configured listen address (with localhost / 127.0.0.1 / [::1] aliases when bound to a local host) plus any hosts explicitly added through httpAllowedOrigins. Requests with a mismatched or missing Host header return 403.
JulienJBO
changed the title
Contributor
Author
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0d804899b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hosts.add(host); | ||
| } | ||
| } else { | ||
| hosts.add(`${config.httpHost}:${port}`.toLowerCase()); |
There was a problem hiding this comment.
allowedHosts() adds only ${config.httpHost}:${port} for non-local binds, so with common deployment settings like --http-host=0.0.0.0 (or ::) every request is denied: clients send a concrete Host (service DNS/IP), never the wildcard bind address itself. In that mode the new Host check returns 403 for valid non-browser traffic unless operators repurpose httpAllowedOrigins as a Host allowlist, which breaks expected HTTP transport behavior.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Résumé
Suite d'un audit ciblé de la surface sensible du serveur (rédaction de secrets, exécution
op, transport HTTP, client Connect). Six correctifs atomiques surcodex/audit-reference-redaction, classés par sévérité décroissante.bb7d7eeop://case-insensitive dans l'audit9b4a1cfError.causenon rédigé dansnormalizeError09aa1d1iajouté àredactExactValuepour matcher les casses mixtesad8ce51sanitizeAuditValuetraverse non-enumerable +Symbolkeys viaReflect.ownKeyse8f4341decodeURIComponenttry/catch +.normalize("NFC")sur les références Connectb0d8048Host(defense-in-depth DNS rebinding)Détail des bugs
1.
Error.causeexfiltrable (9b4a1cf)normalizeErrorpropageait{ cause: error }intact. Un secret présent dans la cause d'origine (URLfetchavec credentials, payload SDK 1Password contenant un token, etc.) traversait la rédaction. La cause est désormais détachée lors de la normalisation. Test danserrors.test.ts.2.
redactExactValuecase-insensitive (09aa1d1)La regex était construite sans flag
i, contrairement àredactAuthTextjuste en dessous. SisecretRedactionValuescontient"MyAPIKey"et queopproduitmyapikey(normalisation URL, message DNS), la valeur fuit. Flag passé à"gi".3.
sanitizeAuditValuesuperficielle (ad8ce51)Utilisait
Object.entries, qui ignore les propriétés non-énumérables et les clésSymbol. Plusieurs SDK (AWS, Octokit, undici) stockent des credentials sur ce type de propriétés. Itération désormais viaReflect.ownKeys+ descripteur de propriété, avec gestion des getters qui throw.4. Parsing Connect (
e8f4341)decodeURIComponentjetait unURIErrorbrut sur les escapes malformés (%E0sans byte de continuation). Enveloppé dans untry/catchqui renvoie uneErrortypée cohérente avec le reste du parseur.equalsIdentifierne normalisait pas l'Unicode. Un labelcafé(NFC, U+00E9) ne matchait pas une requêtecafé(NFD, e + combining acute)..normalize("NFC")ajouté des deux côtés.5. Validation du Host header (
b0d8048)La défense contre DNS rebinding reposait uniquement sur
Origin, qui n'est pas envoyé par les clients non-navigateur. Un processus local malveillant via DNS rebinding pouvait atteindre le socket sans déclencher le check. Ajout deassertHostAllowedqui valide le headerHostcontre l'adresse d'écoute (et les origines configurées). Test forcé vianode:httprequestcarfetchinterdit de fixerHost.Validation
npm run lint✓npm test✓ (147 tests, dont 6 nouveaux pour les correctifs)npx tsc --noEmit✓Hors scope
@1password/sdk@0.4.1-beta.1(beta) — surveiller les release notes pour des nouvelles propriétés d'erreur non rédigées.localhost/127.0.0.1/::1par config — si cette limite saute, refaire un audit SSRF/TLS complet.