fix(tokowaka-client): real browser UA for verifyRouting human probe#1749
fix(tokowaka-client): real browser UA for verifyRouting human probe#1749ABHA61 wants to merge 5 commits into
Conversation
…an probe The human leg of verifyRouting used a bare `Mozilla/5.0`, which bot-detection can misclassify — making the human probe look bot-like and the verify falsely fail. Default to a realistic desktop Chrome User-Agent (overridable via an optional `humanUa` option). The bot probe (chatgpt-user) is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
There was a problem hiding this comment.
Hey @ABHA61,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - clean, well-scoped bug fix with tests; no blocking issues.
Complexity: LOW - small diff, single package, no risk signals.
Changes: Replaces the bare Mozilla/5.0 User-Agent in the human probe of verifyRouting with a realistic Chrome UA string and makes it overridable via an options parameter (2 files).
Non-blocking (2): minor issues and suggestions
- nit: Hardcoded
Chrome/124.0.0.0will age silently - bot-detection systems that flag outdated UA versions could eventually trigger the same false-failure this PR fixes. Consider a comment noting the vintage (e.g.// Last updated 2024-Q2 - bump periodically) so future maintainers know when to refresh. -packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1202 - suggestion: Both new tests rely on
fetchStub.secondCall(positional). If the implementation ever reorders thePromise.allarray or adds an intermediate fetch, both tests silently verify the wrong call. A more robust assertion would find the call whoseuser-agentheader is NOTchatgpt-user. -packages/spacecat-shared-tokowaka-client/test/cdn/cloudfront/index.test.js:2048
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 18s | Cost: $2.32 | Commit: c4e7c40a4d6f4d3c03ff168466eddd55699031d8
If this code review was useful, please react with 👍. Otherwise, react with 👎.
MysticatBot
added
ai-reviewed
…tags/guards - assumeConnectorRole accepts an operator (email) and sets it as the RoleSessionName so the customer's CloudTrail attributes each cross-account mutation to who ran it. - Tag the resources we create (Lambda, its exec role, the CloudFront function) with a fixed owner marker + createdBy; verify ownership before reusing/modifying any same-named resource (origin, cache clone, function, lambda, exec role) and refuse with an actionable "set up manually" error when it is not ours. - data-access: add optional edgeOptimizeConfig.externalId (org-level connector id). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nership-guard branches The org-level externalId persistence (the edgeOptimizeConfig.externalId Joi field in data-access) is superseded by server-side derivation of the connector external ID from the org id in api-service (no persistence needed — see the CloudFront externalId change there), so the field is dead. Remove it. Also add the branch coverage the attributable-assume-role + resource-ownership code needs to keep tokowaka-client at 100% branches: - sessionNameFor with a valid operator (RoleSessionName = operator email) - tagCreatedBy falling back to 'unknown' when the operator sanitizes to empty - the function / cache-policy / Lambda reuse guards when the resource's ownership marker field (FunctionSummary / Comment / Role) is absent tokowaka-client: 970 passing, 100% branches. data-access: unchanged behavior, 2654 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @ABHA61,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - two observability/consistency issues in the new ownership-guard logic.
Complexity: HIGH - large diff (941 lines); ownership infrastructure + operator threading + UA fix in one PR.
Changes: Adds resource ownership tags and guards (ManagedBy/createdBy) across CloudFront, Lambda, and IAM resources to prevent reusing non-owned infrastructure, threads operator identity for CloudTrail attribution, and fixes the verifyRouting human probe User-Agent (2 files).
Scope note: title says "real browser UA for verifyRouting human probe" but the diff also delivers ownership tagging, resource reuse guards, and operator identity threading - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js.
Note: Recommend a human read before merge - possible divergence from the documented design (heuristic, flagged by the conventions reviewer). The bot review is a complement to, not a replacement for, a human read here.
Must fix before merge
- [Important] Silent catch block in best-effort tagging hides all errors without any log signal -
packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:522(details inline) - [Important] Lambda ownership check is bypassed when function is in Pending/InProgress state, causing confusing provisioning-then-throw sequence for non-owned Lambdas -
packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:974(details inline)
Non-blocking (3): minor issues and suggestions
- nit: Duplicate consecutive
if (existing)blocks in createOrigin should be merged into one - the guard and the self-heal logic share the same condition and separating them invites confused maintenance. -packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:373 - nit: Hardcoded
Chrome/124.0.0.0in DEFAULT_HUMAN_UA will age silently. Bot-detection that flags outdated UAs could eventually trigger the same false-failure this PR fixes. The override escapes this, but consider a code comment noting when to refresh (e.g. annually). -packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1297 - suggestion:
verifyRoutingtests rely on positionalfetchStub.secondCallordering. If thePromise.allarray is ever reordered or a retry wrapper added, the tests verify the wrong probe. Find the call by UA header content instead. -packages/spacecat-shared-tokowaka-client/test/cdn/cloudfront/index.test.js:2293
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 50s | Cost: $6.21 | Commit: 18f498e3ed85f1a0aa76b2e0dd0624b62b9837d0
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| * @param {string} [params.operator] - operator identity (email); used as the RoleSessionName so the | ||
| * customer's CloudTrail attributes each mutation to the person who ran it. | ||
| * @returns {Promise<{roleArn: string, accountId: string, credentials: object}>} | ||
| */ |
There was a problem hiding this comment.
issue (blocking): The empty catch swallows ALL exception classes (TypeError, credential expiration, throttling, SDK config errors) without any observable signal. If TagResourceCommand systematically fails (wrong Tags shape, missing permission boundary, invalid ARN), the ownership model silently degrades from "tag + Comment marker" to "Comment marker only" and operators have no way to detect it without manually inspecting CloudFront resource tags in the customer account.
The Comment marker IS a sufficient fallback for ownership decisions, so this is not a safety bug. But silent, invisible failure in production code running in customer accounts is an observability gap that should not ship without at minimum a log line.
Fix: Replace the empty catch with a warn/debug-level log:
} catch (e) {
log.debug(`best-effort tagging of ${functionArn} failed: ${e.message}`);
}If no logger is in scope, at minimum narrow the catch to the expected AccessDeniedException and re-throw unexpected errors.
| @@ -116,13 +160,16 @@ const delay = (ms) => new Promise((resolve) => { | |||
| * @param {string} params.externalId - external ID baked into the connector role trust policy. | |||
There was a problem hiding this comment.
issue (blocking): The assertOwnedOrThrow for the Lambda function (further down at line ~978) runs AFTER the Pending/InProgress early-return. When a non-owned Lambda is still initializing (State=Pending or LastUpdateStatus=InProgress), the function returns { status: "provisioning" } without ever checking ownership. On the next poll (once Active), it throws.
The caller sees "provisioning" (implying our deploy is in progress) then a hard ownership error on the next call. This is confusing UX and a potential false-positive in orchestration that treats "provisioning" as "we are making progress."
The IAM role IS correctly guarded before this point, so safety is preserved (we never publish code into a non-owned function). But the behavioral inconsistency should be resolved.
Fix: Move the ownership check (assertOwnedOrThrow on cfg.Role) above the Pending/InProgress early-return, or document explicitly in a comment why skipping the guard in that state is acceptable (e.g. the role check already ran, the Lambda cannot be published into during Pending).
MysticatBot
added
complexity:high
2 participants
What
The human leg of
verifyRouting(CloudFront Edge Optimize verify) used a bareMozilla/5.0. Bot-detection can misclassify that as a bot, making the human probe look bot-like and the verify step falsely fail (it requires the human leg to NOT be routed to Edge Optimize).Change
humanUaoption (verifyRouting(url, { humanUa })), default-preserving for existing callers.chatgpt-user) is unchanged.Tests
Added two cases (default browser UA is used for the human probe; custom
humanUais honored).verifyRoutingsuite green (11 passing); eslint clean.Consumer
@adobe/spacecat-api-servicecallsverifyRouting(url)and inherits the new default after this is published + the dependency is bumped.🤖 Generated with Claude Code