sneak/upaas
1
0
Fork 1
Code Issues 8 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

lint: fix 22 gosec G710 open-redirect findings in internal/handlers/app.go #176

New Issue
Closed
opened 2026-08-07 18:40:31 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-07 18:40:31 +02:00
Collaborator
Copy Link

Surveyed 2026-08-07: golangci-lint 2.12.2 reports 23 gosec findings
on main; 22 of them are G710: Open redirect via taint analysis, all
in internal/handlers/app.go (lines 289, 374, 400, 424, 429, 802, 835,
889, 899, 1034, 1062, 1075, 1091, 1120, 1148, 1169, 1215, 1277, 1290,
1326, 1334, 1348). The remaining G703 finding is tracked in its own
issue.

Every site has the same shape: http.Redirect with a URL built by
string concatenation from application.ID, which taint analysis
considers request-derived, e.g.:

redirectURL := "/apps/" + application.ID + "?success=updated"
http.Redirect(writer, request, redirectURL, http.StatusSeeOther)

App IDs are ULIDs. Remediation: add one small helper (e.g.
redirectToApp(w, r, appID, query string)) that validates the ID is a
well-formed ULID (or applies url.PathEscape) before building the
relative URL, then convert all 22 sites to use it. This both satisfies
the taint analysis and makes the invariant explicit. Do not use
//nolint — fix the code, per the README commit requirements.

Definition of done:

  • make lint (golangci-lint ≥ 2.12) reports zero G710 findings
  • no //nolint directives added
  • redirect behavior unchanged (make test passes; existing handler
    tests still green)
  • make fmt run before commit; TODO.md updated per the repo workflow
  • lands via PR from a feature branch off main
Surveyed 2026-08-07: golangci-lint 2.12.2 reports 23 `gosec` findings on `main`; 22 of them are `G710: Open redirect via taint analysis`, all in `internal/handlers/app.go` (lines 289, 374, 400, 424, 429, 802, 835, 889, 899, 1034, 1062, 1075, 1091, 1120, 1148, 1169, 1215, 1277, 1290, 1326, 1334, 1348). The remaining `G703` finding is tracked in its own issue. Every site has the same shape: `http.Redirect` with a URL built by string concatenation from `application.ID`, which taint analysis considers request-derived, e.g.: ```go redirectURL := "/apps/" + application.ID + "?success=updated" http.Redirect(writer, request, redirectURL, http.StatusSeeOther) ``` App IDs are ULIDs. Remediation: add one small helper (e.g. `redirectToApp(w, r, appID, query string)`) that validates the ID is a well-formed ULID (or applies `url.PathEscape`) before building the relative URL, then convert all 22 sites to use it. This both satisfies the taint analysis and makes the invariant explicit. Do not use `//nolint` — fix the code, per the README commit requirements. Definition of done: - `make lint` (golangci-lint ≥ 2.12) reports zero `G710` findings - no `//nolint` directives added - redirect behavior unchanged (`make test` passes; existing handler tests still green) - `make fmt` run before commit; `TODO.md` updated per the repo workflow - lands via PR from a feature branch off `main`
clawbot added this to the 1.1.0 milestone 2026-08-07 18:40:31 +02:00
clawbot commented 2026-08-07 18:53:00 +02:00
Author
Collaborator
Copy Link

Starting this now on branch fix-gosec-g710, stacked on
fix-noctx-lint (PR #183) so the lint-count progression and the
TODO.md rotation stay serial; the PR should merge after #183.

Definition of done for the PR:

  • one shared helper in internal/handlers that validates the app ID
    before building the redirect target; all 22 G710 sites converted
    to it
  • make lint (golangci-lint 2.12.2): zero G710 findings; total
    drops 47 → 25 (remaining: 1 gosec G703 → #177, 24 goconst →
    #178)
  • no //nolint directives; redirect behavior unchanged; make test
    green
  • make fmt run; TODO.md Next Step rotated to #177
  • before/after lint counts stated on the PR
Starting this now on branch `fix-gosec-g710`, stacked on `fix-noctx-lint` (PR #183) so the lint-count progression and the `TODO.md` rotation stay serial; the PR should merge after #183. Definition of done for the PR: - one shared helper in `internal/handlers` that validates the app ID before building the redirect target; all 22 `G710` sites converted to it - `make lint` (golangci-lint 2.12.2): zero `G710` findings; total drops 47 → 25 (remaining: 1 `gosec` G703 → #177, 24 `goconst` → #178) - no `//nolint` directives; redirect behavior unchanged; `make test` green - `make fmt` run; `TODO.md` Next Step rotated to #177 - before/after lint counts stated on the PR
clawbot commented 2026-08-07 18:58:31 +02:00
Author
Collaborator
Copy Link

Done in PR #186 (branch fix-gosec-g710, commit b580dbb, stacked on
PR #183): all 22 G710 findings resolved via a redirectToApp
helper (ulid.ParseStrict + re-serialize + url.PathEscape),
make lint total 47 → 25, make test and make fmt-check green.
Awaiting independent review (labeled needs-review); merge after #183.

Done in PR #186 (branch `fix-gosec-g710`, commit b580dbb, stacked on PR #183): all 22 `G710` findings resolved via a `redirectToApp` helper (`ulid.ParseStrict` + re-serialize + `url.PathEscape`), `make lint` total 47 → 25, `make test` and `make fmt-check` green. Awaiting independent review (labeled needs-review); merge after #183.
clawbot commented 2026-08-07 22:27:19 +02:00
Author
Collaborator
Copy Link

Definition of done verified met on main (7a34fc9) — the fix landed
via #187 rather than PR #186, which is now closed as superseded:

  • all 22 G710 sites in internal/handlers/app.go now go through a
    redirectToApp helper that builds
    "/apps/" + url.PathEscape(appID) + suffix — a real sanitization
    producing an always-relative application URL (url.PathEscape is
    one of the two remediations this issue accepts)
  • no //nolint or #nosec directives added for these sites
  • make lint (pinned golangci-lint v2.12.2, canonical config): 0
    findings, so zero G710
  • make check green in a clean worktree of main (tests with race
    detector, lint, fmt-check); redirect behavior unchanged, handler
    tests green

Closing.

Definition of done verified met on `main` (7a34fc9) — the fix landed via #187 rather than PR #186, which is now closed as superseded: - all 22 `G710` sites in `internal/handlers/app.go` now go through a `redirectToApp` helper that builds `"/apps/" + url.PathEscape(appID) + suffix` — a real sanitization producing an always-relative application URL (`url.PathEscape` is one of the two remediations this issue accepts) - no `//nolint` or `#nosec` directives added for these sites - `make lint` (pinned golangci-lint v2.12.2, canonical config): 0 findings, so zero `G710` - `make check` green in a clean worktree of `main` (tests with race detector, lint, `fmt-check`); redirect behavior unchanged, handler tests green Closing.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
 notplanned
question
More information is needed
 wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone 1.1.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/upaas#176
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?