Skip to content

fix: replace ternary side effect with if/else and fix null Authorizat…#10931

Merged
jasonsaayman merged 4 commits into
axios:v1.xfrom
azandabot:fix/github-token-style-and-null-header
Jun 1, 2026
Merged

fix: replace ternary side effect with if/else and fix null Authorizat…#10931
jasonsaayman merged 4 commits into
axios:v1.xfrom
azandabot:fix/github-token-style-and-null-header

Conversation

@azandabot
Copy link
Copy Markdown
Contributor

@azandabot azandabot commented May 21, 2026

Summary

Two issues found in scripts/axios-build-instance.js:

  1. Ternary used for side effects
    The contributing guidelines reference the node-style-guide which discourages using ternary operators purely for side effects with no return value. Replaced with an if/else for clarity.

  2. Null Authorization header
    When GITHUB_TOKEN is undefined, the header was being set to null rather than being omitted entirely. Setting a header to null can result in the string "null" being sent in some environments. Fixed using conditional object spreading so the header is simply absent when no token is provided.

Note: npm run test shows 1049/1050 tests passing. The single unhandled error is a webkit browser session timeout unrelated to this change.


Summary by cubic

Fixes the GitHub axios client to omit the Authorization header when GITHUB_TOKEN is missing and replaces a ternary side effect with a clear if/else. Prevents sending a "null" header; behavior is unchanged when a token is provided.

Changes

  • In scripts/axios-build-instance.js, replace ternary side effect with if/else for logging.
  • Only set Authorization when GITHUB_TOKEN exists using conditional object spread.
  • Synced with latest v1.x (no functional changes).

Notes

  • Docs: Update /docs/ to clarify the header is added only when a token is set.
  • Testing: No tests added; add cases to assert header presence/absence and log paths.
  • Semantic version impact: Patch.

Written for commit b525733. Summary will update on new commits.

Review in cubic

@azandabot azandabot requested a review from jasonsaayman as a code owner May 21, 2026 20:02
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 1 file

Confidence score: 4/5

  • This PR is likely safe to merge, with only a moderate-risk behavior change noted (severity 5/10, confidence 7/10).
  • In scripts/axios-build-instance.js, conditionally omitting Authorization may stop explicitly clearing inherited default headers, which can lead to unintended auth headers being sent in some request paths.
  • The impact appears scoped to header-construction behavior rather than a broad functional break, so risk looks manageable with a focused verification pass.
  • Pay close attention to scripts/axios-build-instance.js - ensure Authorization is explicitly unset when required to avoid inherited default header leakage.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread scripts/axios-build-instance.js Outdated
@jasonsaayman jasonsaayman added priority::medium A medium priority commit::fix The PR is related to a bugfix labels May 24, 2026
@azandabot azandabot force-pushed the fix/github-token-style-and-null-header branch from fb4594a to f42d845 Compare May 25, 2026 07:14
Copy link
Copy Markdown
Member

@jasonsaayman jasonsaayman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple changes before this one can land.

Comment thread scripts/axios-build-instance.js Outdated
Comment thread scripts/axios-build-instance.js Outdated
@jasonsaayman jasonsaayman added the status::changes-requested A reviewer requested changes to the PR label May 27, 2026
@azandabot
Copy link
Copy Markdown
Contributor Author

Done, both changes applied. Thanks for the review!

@jasonsaayman jasonsaayman dismissed their stale review May 27, 2026 18:51

All threads resolved

@jasonsaayman jasonsaayman removed the status::changes-requested A reviewer requested changes to the PR label Jun 1, 2026
@jasonsaayman jasonsaayman merged commit 32e2515 into axios:v1.x Jun 1, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit::fix The PR is related to a bugfix priority::medium A medium priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants