Skip to content

tool_operate: reset the URL --url-query between --next#20802

Closed
bagder wants to merge 2 commits intomasterfrom
bagder/url-query
Closed

tool_operate: reset the URL --url-query between --next#20802
bagder wants to merge 2 commits intomasterfrom
bagder/url-query

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Mar 3, 2026

Pointed out by Codex Security

Verify in test 1624

@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 3, 2026

@aisle-analyzer augment review

@aisle-research-bot
Copy link
Copy Markdown

aisle-research-bot bot commented Mar 3, 2026

🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.

Aisle supplements but does not replace security review.


Analyzed PR: #20802 at commit 47c8a0f

Last updated on: 2026-03-03T18:11:33Z

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Mar 3, 2026

🤖 Augment PR Summary

Summary: This PR fixes --url-query state leaking across --next transfers by resetting the per-transfer query fields in the tool layer.

Changes:

  • Reset `global->state.httpgetfields` at the start of each `single_transfer()` so a prior transfer’s `--url-query` does not carry into the next operation.
  • Adjust `single_transfer()` initialization so query state is rebuilt from the current config each time.
  • Add new test case test1624 that exercises --url-query + --next and verifies the query is only applied to the first request.
  • Register the new test in tests/data/Makefile.am.

Technical Notes: The new test validates that a sensitive query token is not inadvertently reused on the subsequent URL after --next.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 1 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

@testclutch
Copy link
Copy Markdown

Analysis of PR #20802 at 47c8a0f9:

Test 48 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 280 different CI jobs (the link just goes to one of them).

Test 199 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 250 different CI jobs (the link just goes to one of them).

Test 2500 failed, but it has been 60.3% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (3, to be exact) so it may be that this failure is rather a systemic issue with this job and not with this specific PR.

Generated by Testclutch

bagder added 2 commits March 3, 2026 22:50
Pointed out by Codex Security

Verify in test 1624
To make it reset properly after --next, but not before
@bagder bagder force-pushed the bagder/url-query branch from 47c8a0f to 5fe5a3b Compare March 3, 2026 21:50
@bagder bagder requested review from Copilot March 3, 2026 22:17
@bagder bagder marked this pull request as ready for review March 3, 2026 22:17
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a state-leak bug in tool_operate where --url-query could incorrectly carry over across --next operations (as flagged by Codex Security), and adds a regression test to ensure query parameters are properly reset between operations.

Changes:

  • Move httpgetfields from global State to per-OperationConfig so it does not persist across --next.
  • Update URL query appending logic to use config->httpgetfields instead of state->httpgetfields.
  • Add test 1624 and register it in the test suite to verify correct behavior.

Reviewed changes

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

File Description
src/tool_operate.c Switches query-append source from global state to per-operation config, preventing cross---next leakage.
src/tool_cfgable.h Moves httpgetfields storage to OperationConfig (removes it from State).
tests/data/test1624 New regression test verifying --url-query applies only to the first operation before --next.
tests/data/Makefile.am Adds test1624 to the test list so it runs in CI.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@bagder bagder closed this in 933c34e Mar 6, 2026
@bagder bagder deleted the bagder/url-query branch March 6, 2026 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants