Skip to content

Require HCA step-up before account deletion - #1532

Open
skyfallwastaken wants to merge 1 commit into
mainfrom
require-hca-step-up-for-deletion
Open

Require HCA step-up before account deletion#1532
skyfallwastaken wants to merge 1 commit into
mainfrom
require-hca-step-up-for-deletion

Conversation

@skyfallwastaken

Copy link
Copy Markdown
Member

Summary of the problem

Accounts linked to Hack Club Auth could submit an account deletion request without confirming control of the linked HCA identity. Account deletion is a sensitive operation and should require HCA step-up authentication when that link exists.

Describe your changes

Linked accounts now enter an HCA OpenID Connect flow with prompt=login before a deletion request is created. The callback uses single-use state bound to the initiating Hackatime user and linked HCA ID, requires the returned identity to match exactly and rechecks deletion eligibility before continuing.

Accounts without HCA retain the existing deletion flow. Development setup now documents the additional callback URL.

Before deployment, https://hackatime.hackclub.com/deletion/hca/callback must be registered on the production HCA OAuth client.

Screenshots / Media

No visual changes.

Copilot AI review requested due to automatic review settings August 7, 2026 12:27

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds Hack Club Auth step-up verification before linked users can create deletion requests, while preserving direct creation for unlinked users.

  • Stores a single-use, user- and HCA-identity-bound pending deletion request in the session.
  • Adds an HCA callback that exchanges the authorization code, verifies the returned identity, and rechecks deletion eligibility.
  • Generalizes HCA authorization URL construction and adds token-to-HCA-ID exchange support.
  • Documents the additional callback URL and adds controller coverage for success, mismatch, invalid state, replay, identity changes, and eligibility changes.

Confidence Score: 3/5

The PR should not merge until oversized deletion details and HCA transport or parsing failures are handled without crashing the deletion flow.

Linked users can currently trigger a cookie overflow through unrestricted deletion details, and ordinary external HCA failures escape the callback as unhandled server errors after pending state has been consumed.

Files Needing Attention: app/controllers/deletion_requests_controller.rb and app/models/concerns/oauth_authentication.rb

Important Files Changed

Filename Overview
app/controllers/deletion_requests_controller.rb Adds the step-up flow and robust state/identity checks, but stores unbounded input in the cookie session and does not handle external-service failures.
app/models/concerns/oauth_authentication.rb Generalizes HCA authorization parameters and adds token exchange logic whose transport and parsing failures propagate to callers.
config/routes.rb Adds the browser callback route required for the HCA deletion step-up.
test/controllers/deletion_requests_controller_test.rb Covers the principal success and rejection paths but omits oversized session input and failed HCA HTTP responses.
DEVELOPMENT.md Documents both local HCA callback URLs required for development.

Sequence Diagram

sequenceDiagram
  participant User
  participant App as Hackatime
  participant HCA as Hack Club Auth
  User->>App: POST /deletion
  alt HCA identity linked
    App->>App: Store pending request and state
    App-->>User: "Redirect to HCA with prompt=login"
    User->>HCA: Authenticate
    HCA-->>App: GET callback with code and state
    App->>App: Validate state, user, and linked HCA ID
    App->>HCA: Exchange authorization code
    HCA-->>App: Access token
    App->>HCA: Fetch identity
    HCA-->>App: identity.id
    App->>App: Match identity and recheck eligibility
    App->>App: Create deletion request
  else No HCA identity linked
    App->>App: Create deletion request immediately
  end
Loading
Prompt To Fix All With AI
### Issue 1
app/controllers/deletion_requests_controller.rb:85
**Session cookie overflows on details**

When a linked-HCA user submits sufficiently long `reason_details`, `begin_hca_step_up` copies the unrestricted value into the cookie-backed session, exceeding its size limit and causing the redirect to fail with `ActionDispatch::Cookies::CookieOverflow` before step-up can begin.

### Issue 2
app/controllers/deletion_requests_controller.rb:40
**HCA exchange failures escape callback**

If HCA times out, is unreachable, or returns a non-JSON error response, `hca_id_from_token` lets the transport or parsing exception escape, causing a 500 response after `pending_deletion_request` has already been consumed and forcing the user to restart deletion.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "Require HCA step-up for account deletion" | Re-trigger Greptile

"state" => state,
"user_id" => current_user.id,
"hca_id" => current_user.hca_id,
"attributes" => deletion_request_params.stringify_keys

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.

P1 Session cookie overflows on details

When a linked-HCA user submits sufficiently long reason_details, begin_hca_step_up copies the unrestricted value into the cookie-backed session, exceeding its size limit and causing the redirect to fail with ActionDispatch::Cookies::CookieOverflow before step-up can begin.

Knowledge Base Used: Authentication: sessions, API keys, and OAuth

Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/deletion_requests_controller.rb
Line: 85

Comment:
**Session cookie overflows on details**

When a linked-HCA user submits sufficiently long `reason_details`, `begin_hca_step_up` copies the unrestricted value into the cookie-backed session, exceeding its size limit and causing the redirect to fail with `ActionDispatch::Cookies::CookieOverflow` before step-up can begin.

**Knowledge Base Used:** [Authentication: sessions, API keys, and OAuth](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/api-authentication.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

end

redirect_uri = hca_deletion_callback_url
hca_id = User.hca_id_from_token(params[:code], redirect_uri)

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.

P1 HCA exchange failures escape callback

If HCA times out, is unreachable, or returns a non-JSON error response, hca_id_from_token lets the transport or parsing exception escape, causing a 500 response after pending_deletion_request has already been consumed and forcing the user to restart deletion.

Knowledge Base Used: Authentication: sessions, API keys, and OAuth

Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/deletion_requests_controller.rb
Line: 40

Comment:
**HCA exchange failures escape callback**

If HCA times out, is unreachable, or returns a non-JSON error response, `hca_id_from_token` lets the transport or parsing exception escape, causing a 500 response after `pending_deletion_request` has already been consumed and forcing the user to restart deletion.

**Knowledge Base Used:** [Authentication: sessions, API keys, and OAuth](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/api-authentication.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

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.

2 participants