FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix warning for PAT-like token with username by mbg · Pull Request #3579 · github/codeql-action · GitHub

/ codeql-action Public

Fix warning for PAT-like token with username - #3579

Merged
mbg merged 11 commits into
mainfrom
mbg/start-proxy/token-check-fixes
Mar 25, 2026
ConversationCommits (11)ChecksFiles changed
Merged

Fix warning for PAT-like token with username#3579
mbg merged 11 commits into
mainfrom
mbg/start-proxy/token-check-fixes

Conversation

mbg commented Mar 16, 2026

Copy link
Copy Markdown
Member

This was missed in #3563 and caught in #3577 (comment).

Risk assessment

For internal use only. Please select the risk level of this change:

  • Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Workflow types:

  • Managed - Impacts users with dynamic workflows (Default Setup, Code Quality, ...).

Products:

  • Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
  • Code Quality - The changes impact analyses when analysis-kinds: code-quality.
  • Other first-party - The changes impact other first-party analyses.

Environments:

  • Dotcom - Impacts CodeQL workflows on github.com and/or GitHub Enterprise Cloud with Data Residency.
  • GHES - Impacts CodeQL workflows on GitHub Enterprise Server.

How did/will you validate this change?

  • Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
  • End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

  • Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

  • Telemetry - I rely on existing telemetry or have made changes to the telemetry.
    • Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.
    • Alerts - New or existing monitors will trip if something goes wrong with this change.

Are there any special considerations for merging or releasing this change?

  • No special considerations - This change can be merged at any time.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Consider adding a changelog entry for this change.
  • Confirm the readme and docs have been updated if necessary.

mbg requested a review from a team as a code owner March 16, 2026 19:36
Copilot AI review requested due to automatic review settings March 16, 2026 19:36
github-actions Bot added the size/M Should be of average difficulty to review label Mar 16, 2026
Copilot AI reviewed Mar 16, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull request overview

This PR fixes an incorrect warning emitted by start-proxy when credentials use a PAT-like token with a configured username, and expands unit test coverage to validate the corrected behavior.

Changes:

  • Adjust PAT-warning logic in getCredentials to only warn when a PAT is used and no username is provided.
  • Refactor/add test utilities (RecordingLogger.hasMessage, assertNotLogged) and expand getCredentials tests to cover username/no-username combinations.
  • Add a VS Code snippet file (unrelated to the warning fix).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/start-proxy.ts Fixes the PAT warning condition to account for username presence for both token and password configs.
src/start-proxy.test.ts Adds macro-based tests covering PAT warning behavior with/without usernames for token and password inputs.
src/testing-utils.ts Enhances the recording logger with message-search helpers and adds a negative-log assertion helper.
lib/start-proxy-action.js Generated output reflecting the src/start-proxy.ts change.
.vscode/tests.code-snippets Adds an editor snippet (not directly related to the PR’s stated purpose).

Comment thread src/testing-utils.ts
Comment thread src/testing-utils.ts
Comment thread src/start-proxy.test.ts Outdated
Comment thread .vscode/tests.code-snippets
esbena reviewed Mar 17, 2026
View reviewed changes

esbena left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Thanks for the thorough testing improvements.

Comment thread src/testing-utils.ts
/**
* Asserts that `message` should not have been logged to `logger`.
*/
export function assertNotLogged(

esbena Mar 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Two minor things on this one:

  1. Naming: doesn't follow the check... naming convention of checkExpectedLogMessages

  2. Could be upgraded to (string | RegExp)[] since we already uses hasLoggedMessage under the hood

mbg Mar 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To be honest, I just lazily picked the commits with the testing utils changes from #3508 since assertNotLogged was useful here and I figured that I probably wouldn't get around to polishing that PR more otherwise 😅

For 1: I am planning to move the names away from check... to assert.... I don't think I want to replace all occurrences of checkExpectedLogMessages now though as part of this PR. Personally, I am OK with the naming inconsistency, since we already have assert... for other assertions and the odd one out is checkExpectedLogMessages.

esbena previously approved these changes Mar 17, 2026
View reviewed changes

esbena left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Approving, but please see my thoughts on assertNotLogged

mbg mentioned this pull request Mar 25, 2026
Closed
mbg enabled auto-merge March 25, 2026 12:58
mbg added this pull request to the merge queue Mar 25, 2026
Merged via the queue into main with commit 3d564d9 Mar 25, 2026
227 checks passed
mbg deleted the mbg/start-proxy/token-check-fixes branch March 25, 2026 13:27
This was referenced Mar 27, 2026
Closed
Merged
Merged
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
esbena esbena approved these changes

Assignees

No one assigned

Labels

size/M Should be of average difficulty to review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL