Skip to content

fix(textract): normalise env-derived config so quoted vars cannot poison the SDK - #274

Open
mashhoodr wants to merge 1 commit into
developfrom
fix-textract-env-quote-stripping
Open

fix(textract): normalise env-derived config so quoted vars cannot poison the SDK#274
mashhoodr wants to merge 1 commit into
developfrom
fix-textract-env-quote-stripping

Conversation

@mashhoodr

Copy link
Copy Markdown
Contributor

What broke

NIETE ran with AWS_REGION_TEXTRACT = "ap-southeast-1" — the region with literal double-quote characters, 16 bytes for a 14-byte region. AWS_TEXTRACT_ACCESS_KEY_ID (22 chars for a 20-char key) and AWS_TEXTRACT_SECRET_ACCESS_KEY (42 for 40) were quoted the same way, on both the bot and sqs-worker services — the signature of one block paste, not a typo.

@smithy's validator formats region="${region}", which is why the error read with doubled quotes:

Region not accepted: region=""ap-southeast-1"" is not a valid hostname component.
    at checkRegion (@smithy/core/.../checkRegion.js:9)
    at AWSTextractService.extractText (bot/shared/services/aws-textract.service.js:58)
    at textractFallback (bot/workers/lesson-plan-extraction.worker.js:219)
    at SQSCoachingWorker.executeJob (bot/workers/sqs-worker.js:345)

Textract is the lesson-plan OCR fallback, and that throw propagates, so it terminated the coaching session. Verified identical in 30/30 occurrences: lesson_plan_extraction_status='failed' on all 30, and 23 teachers never received a report.

Because the credentials were quoted too, this path had never worked since the day it was configured. It only runs for lesson plans whose text layer is too thin for pdf-parse, so it stayed latent until traffic grew enough to hit that case regularly.

Why it's not a one-line fix at the call site

A .env file would have been fine — dotenv strips wrapping quotes when parsing. A platform env var (Railway, Fly, a k8s secret) is delivered verbatim, so quotes pasted into a dashboard become part of the value. That asymmetry is the real bug and it applies to any env-derived config, not just this one.

Note the AWS_TEXTRACT_* override pair is what let it hide: it shadows a perfectly good generic AWS_* config. The generic AWS_REGION was clean (us-east-1) the whole time — which is exactly why SQS never failed and only Textract did.

The change

bot/shared/utils/env.jsenvStr(name):

  • trims, then removes one layer of balanced quotes
  • returns undefined for unset/empty, so existing A || B chains behave identically
  • warns once per variable instead of repairing silently — the operator needs to learn which var is malformed
  • never throws — config normalisation must not become a new way to fail at boot
  • leaves unbalanced quotes alone: intent is unknowable, and inventing a value is worse than failing loudly
  • leaves JSON values intact (REGION_FRAMEWORK_MAP is pinned by a test)

Applied to the region and the credentials, including assertTextractCredentialsPresent() — reading raw process.env there would let a value of "" pass as "present" while the SDK receives undefined, and a check that disagrees with what it guards produces the worst class of error report.

Verification

  • 20 new tests in tests/config/env-value.test.js, all passing
  • tests/config + tests/setup (655 tests): the same 14 suite failures before and after — all pre-existing (dangling markdown link, schema conformance, etc.), zero regressions
  • The live variables have already been corrected in Railway and the region error stopped at 07:13 UTC on 2026-08-18. This PR is the durable guard so it cannot recur.

Not in this PR

Two things surfaced during the investigation, deliberately left out:

  1. A second bug the region error was masking. With Textract now reachable, PDF lesson plans return UnsupportedDocumentException: Request has unsupported document format — synchronous AnalyzeDocument with Document: { Bytes } does not accept PDF. First seen 07:17 UTC 2026-08-18, never before, because nothing ever got this far. 26 of the 30 affected sessions were PDFs, so this still needs its own fix.
  2. An optional fallback shouldn't kill the session. 23 teachers lost their report because an optional OCR fallback threw. Worth fixing independently — the next Textract outage (throttle, expired key, AWS incident) would do it again.

🤖 Generated with Claude Code

…son the SDK

NIETE ran with AWS_REGION_TEXTRACT set to the 16-character value
`"ap-southeast-1"` — the region WITH literal double quotes — and both
AWS_TEXTRACT_ACCESS_KEY_ID (22 chars for a 20-char key) and
AWS_TEXTRACT_SECRET_ACCESS_KEY (42 for 40) quoted the same way, on both the
bot and sqs-worker services. @smithy rejected the region
(`Region not accepted: region=""ap-southeast-1""`), so every Textract call
threw. Because Textract is the lesson-plan OCR fallback and that throw
propagates, it terminated the coaching session: 30 sessions hit it and 23
teachers never received their report.

A .env file would have been fine — dotenv strips wrapping quotes when it
parses a file. A platform env var is delivered verbatim, so quotes pasted
into a dashboard become part of the value. That asymmetry is the bug, and it
is not specific to this call site.

envStr() trims and removes ONE layer of balanced quotes, returning undefined
for empty so the existing `A || B` fallback chains behave identically. It
warns once per variable rather than repairing silently, because the operator
needs to learn which var is malformed; and it does not throw, because config
normalisation must not become a new way to fail at boot. Unbalanced quotes
are left alone — intent is unknowable, and inventing a value is worse than
failing loudly.

Applied to the region AND the credentials, including
assertTextractCredentialsPresent(): using raw process.env there would let a
value of `""` pass as "present" while the SDK receives undefined, and a check
that disagrees with what it guards produces the worst error reports.

Note the override pair is what let this hide: AWS_TEXTRACT_* shadows a
perfectly good generic AWS_* config, so nothing inspecting the generic vars
could see it. The generic AWS_REGION was clean the whole time, which is why
SQS never failed and only Textract did.

Verified: 20 new tests pass; tests/config + tests/setup show the same 14
pre-existing suite failures before and after, so no regressions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant