Skip to content

Remove unused Forge GitHub create and OAuth integration - #15812

Merged
borinquenkid merged 2 commits into
8.0.xfrom
fix/forge-oauth-state-validation
Aug 6, 2026
Merged

Remove unused Forge GitHub create and OAuth integration#15812
borinquenkid merged 2 commits into
8.0.xfrom
fix/forge-oauth-state-validation

Conversation

@jamesfredley

@jamesfredley jamesfredley commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Rewrites this PR from OAuth-state hardening to full removal of the unused Forge server-side GitHub create / OAuth / push integration.

The start.grails.org UI removed "Push to GitHub" in grails-forge-ui#61 (2025-07). The server endpoints and OAuth app wiring were left behind. Maintaining them (including state validation) is unnecessary attack surface.

What was removed

  • GitHubCreateController and related create/OAuth/redirect services, DTOs, clients, and tests
  • GitHubUtil (JGit push helper) and the org.eclipse.jgit dependency
  • GrailsForgeConfiguration.GitHubConfiguration and application.yml OAuth/API client config
  • JGit native-image metadata
  • Cloud Run deploy env vars for the OAuth app (GITHUB_OAUTH_APP_*, GITHUB_USER_AGENT), with --remove-env-vars so existing services drop them on next deploy
  • Threat-model references to the deleted controller

What was kept

  • Generated-app GitHub Actions workflow features (feature/github/workflows/**)
  • GITHUB_REDIRECT_URL / grails.forge.redirect-url for browser redirect to the Forge UI

Additional fix

  • GrailsForgeConfiguration.setRedirectUrl previously self-assigned and ignored config; it now applies the provided URL (covered by GrailsForgeConfigurationSpec)

Test plan

  • ./gradlew :grails-forge-api:test :grails-forge-core:test :grails-forge-web-netty:compileJava (from grails-forge/)
  • GrailsForgeConfigurationSpec for redirect override

Follow-ups (ops / UI)

  • Revoke/delete the unused GitHub OAuth App credentials after deploy
  • Optional: scrub dead UI residue (getGitHubLink / callback query handling) in grails-forge-ui

Copilot AI review requested due to automatic review settings July 2, 2026 14:26

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.

Pull request overview

This PR strengthens the Grails Forge GitHub OAuth flow by persisting the OAuth state in a path-scoped HttpOnly cookie and validating it on callback endpoints before proceeding, including support for multiple outstanding states.

Changes:

  • Add state generation, persistence, validation, and consumption via a path-scoped OAuth state cookie.
  • Enforce state validation for both normal and error callbacks, rejecting missing/mismatched state before exchanging OAuth codes.
  • Add a new Spock spec covering redirect/callback state cookie behavior, including multiple outstanding flows.

Reviewed changes

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

File Description
grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy Adds test coverage for OAuth state cookie issuance, validation, and consumption across redirect/callback scenarios.
grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java Introduces cookie construction/expiry/consumption helpers and wires state into the GitHub authorize URL.
grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java Applies state validation and cookie mutation rules to the create endpoint and the OAuth error callback endpoint.

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

throw e;
} else {
return HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeErrorRedirectUrl(e.getMessage()));
return consumeOAuthStateCookie(HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeErrorRedirectUrl(e.getMessage())), requestInfo, state);
if (!hasValidOAuthState(state)) {
return rejectInvalidOAuthState(redirectService.getLauncherURI());
}
redirect = redirectService.constructGrailsForgeErrorRedirectUrl(errorDescription);
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.3431%. Comparing base (e88612c) to head (a1e526f).
⚠️ Report is 6 commits behind head on 8.0.x.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #15812        +/-   ##
==================================================
- Coverage     52.3514%   52.3431%   -0.0083%     
+ Complexity      18299      18296         -3     
==================================================
  Files            2036       2036                
  Lines           96347      96347                
  Branches        16829      16829                
==================================================
- Hits            50439      50431         -8     
- Misses          38485      38492         +7     
- Partials         7423       7424         +1     

see 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jdaugherty

Copy link
Copy Markdown
Contributor

Didn't we remove the ui part of create in github? Why not just remove the function completely

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

It's my understanding we removed the ability to deploy to a github repo. We should remove the functionality rather than maintain it.

@jamesfredley

Copy link
Copy Markdown
Contributor Author

Worth clarifying the current state, because "we removed the ability to deploy to a github repo" is only half-true in this repo - and the half that's left is the risky one.

What was removed is the UI: the "Create in GitHub" action lives in the separate grails-forge-ui (React) repo, which is where that removal happened. But the backend GitHub create/deploy flow is still fully present and wired here in grails-core:

  • grails-forge-api: .../create/github/GitHubCreateController (routes /github/{type}/{name} taking code/state), GitHubRedirectService (builds the OAuth authorize URL with client_id/redirect_uri/scope/state), GitHubCreateService (token exchange -> repo creation -> push), GitHubCreateOperation.
  • grails-forge-core: .../client/github/oauth/GitHubOAuthClient + GitHubOAuthOperations, GitHubUtil.
  • grails-forge-web-netty/src/main/resources/application.yml: live github-oauth / github-api-v3 service URLs and the OAuth client-id/secret/redirect config.

And critically, the state is generated (UUID.randomUUID()) but not validated server-side - which is exactly the CSRF gap this PR closes.

So we currently ship live, reachable OAuth endpoints with an unhardened state. That leaves two coherent options - and the current in-between (UI gone, backend live + unhardened) is the worst of the three:

  1. Retire it fully - remove the backend too: the grails-forge-api .../create/github package, the grails-forge-core GitHub OAuth client, and the application.yml GitHub config, matching the UI removal. That's a separate, larger removal PR.
  2. Keep and harden - if those endpoints stay shipped, they're live CSRF surface and the state validation here is the correct fix.

I'm happy either way. If the decision is to retire, I'll convert this into the backend-removal PR instead. But if any of those /github/... endpoints remain in a released Forge, I'd merge the state validation first so we're not shipping an unhardened OAuth callback. Which direction do you want?

@jdaugherty

Copy link
Copy Markdown
Contributor

@jamesfredley right, we didn't remove in forge backend, but why are we maintaining instead of removing it?

@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
@borinquenkid borinquenkid added this to the grails:8.0.0-RC1 milestone Jul 28, 2026
@jamesfredley

Copy link
Copy Markdown
Contributor Author

This was removed from the UI a long time ago and just needs to be removed from the server side. It is not used.

@jamesfredley
jamesfredley marked this pull request as draft July 29, 2026 18:04
The start.grails.org UI removed Push to GitHub (grails-forge-ui#61).
Delete the unused server-side create/OAuth/push stack, related config,
JGit dependency, native-image metadata, and deployment secrets so the
API no longer accepts GitHub OAuth create flows.

Keep generated-app GitHub Actions workflow features and the browser
redirect URL used by the Forge UI.

Also fix setRedirectUrl so GITHUB_REDIRECT_URL actually overrides the
default, and purge retired OAuth env vars from Cloud Run on deploy.

Assisted-by: Sisyphus:xai/grok-4.5
@jamesfredley jamesfredley changed the title Validate Forge GitHub OAuth state Remove unused Forge GitHub create and OAuth integration Jul 29, 2026
@jamesfredley
jamesfredley force-pushed the fix/forge-oauth-state-validation branch from 6db0745 to bfa1f8e Compare July 29, 2026 22:00
@jamesfredley
jamesfredley marked this pull request as ready for review July 29, 2026 22:32
@jamesfredley

Copy link
Copy Markdown
Contributor Author

Cleaning this up once and for all on both ends.

Backend (this PR): remove the unused Forge GitHub create / OAuth / push stack that the API still exposed after the UI dropped the button.

Frontend companion: apache/grails-forge-ui#159 scrubs the leftover Launch UI residue (github href helpers, OAuth callback routing, clone NextSteps, createRepo tooltip).

Together these match the UI change from grails-forge-ui#61 so neither side keeps a half-dead create-on-GitHub path.

@testlens-app

testlens-app Bot commented Aug 6, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: a1e526f
▶️ Tests: 54560 executed
⚪️ Checks: 61/61 completed


Learn more about TestLens at testlens.app.

@borinquenkid
borinquenkid self-requested a review August 6, 2026 19:13

@borinquenkid borinquenkid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified all review feedback is addressed:

  • jdaugherty's changes-requested (2026-07-09: "remove the functionality rather than maintain it") is resolved — the PR was rewritten (commit bfa1f8e, 2026-07-29) from OAuth-state hardening to full removal of the unused Forge GitHub create/OAuth/push backend, matching that request.
  • Copilot's 3 inline comments all targeted code in GitHubCreateController.java and GitHubRedirectService.java, both of which are now fully deleted in the diff — the flagged issues no longer exist.

Approving; jdaugherty to re-review given the rewrite.

@borinquenkid
borinquenkid merged commit ac94b25 into 8.0.x Aug 6, 2026
63 of 65 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in Apache Grails Aug 6, 2026
@borinquenkid
borinquenkid deleted the fix/forge-oauth-state-validation branch August 6, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants