Skip to content

app/vlselect: restrict log deletion APIs to POST - #1641

Open
func25 wants to merge 1 commit into
masterfrom
post-delete-fix
Open

app/vlselect: restrict log deletion APIs to POST#1641
func25 wants to merge 1 commit into
masterfrom
post-delete-fix

Conversation

@func25

@func25 func25 commented Jul 29, 2026

Copy link
Copy Markdown
Member

Restricts /delete/run_task and /internal/delete/run_task to the POST method to prevent GET-based SSRF attacks from deleting logs.

No "update note" is added as the deletion API is rarely used, migration only requires changing the method to POST, and rejected requests return a clear error explaining the required method. And security note is also highlighted at the top so the same as update note.

Fixes #1635

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 4 files

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

Re-trigger cubic

Comment thread app/vlselect/main.go Outdated
Comment thread app/vlselect/internalselect/internalselect.go Outdated
@func25
func25 requested review from jiekun and makasim July 29, 2026 09:24
@cuongleqq cuongleqq self-assigned this Jul 29, 2026
@jiekun

jiekun commented Jul 29, 2026

Copy link
Copy Markdown
Member

Previously when checking the PR for metrics I was thinking if we should move:

http.Error(w, fmt.Sprintf("Only POST method is allowed. Got %s.", r.Method), http.StatusMethodNotAllowed)

to a "public place". Do you have good candidate for it? If not, it's fine to keep the current one and replace later.

@cuongleqq cuongleqq 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.

Looks good. Since this is a security issue, can we add a small apptest test case to avoid regressions later?

@func25
func25 force-pushed the post-delete-fix branch from e640a1c to 7be7f23 Compare July 29, 2026 19:03
@func25

func25 commented Jul 29, 2026

Copy link
Copy Markdown
Member Author
  • move the check to the exact handler, and keep the message for public-facing changes so users know what happened. this could be inconsistent with how the project handles incorrect http methods.
  • add apptest coverage

@func25
func25 requested a review from cuongleqq July 29, 2026 19:07
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.

deletion API: deleting logs should not be allowed via GET

3 participants