From e4711321c0b2da9ac116802f9ec8647f6743f884 Mon Sep 17 00:00:00 2001 From: James Fredley Date: Thu, 2 Jul 2026 10:22:24 -0400 Subject: [PATCH 1/2] Validate Forge GitHub OAuth state Persist generated GitHub OAuth state in a path-scoped HttpOnly cookie and validate callbacks before exchanging GitHub codes. Reject missing or mismatched callback states without mutating outstanding OAuth state cookies, and consume only the returned valid state so concurrent flows can continue. Assisted-by: Hephaestus:openai/gpt-5.5 codex-review review-gate --- .../create/github/GitHubCreateController.java | 90 ++++- .../create/github/GitHubRedirectService.java | 136 ++++++- .../create/github/GitHubOAuthStateSpec.groovy | 346 ++++++++++++++++++ 3 files changed, 554 insertions(+), 18 deletions(-) create mode 100644 grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy diff --git a/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java b/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java index b217f4ccb0d..d3e179bf856 100644 --- a/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java +++ b/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java @@ -25,10 +25,13 @@ import io.micronaut.http.HttpHeaders; import io.micronaut.http.HttpResponse; import io.micronaut.http.MediaType; +import io.micronaut.http.MutableHttpResponse; import io.micronaut.http.annotation.Controller; import io.micronaut.http.annotation.Get; import io.micronaut.http.annotation.Header; import io.micronaut.http.annotation.QueryValue; +import io.micronaut.http.context.ServerRequestContext; +import io.micronaut.http.cookie.Cookie; import io.micronaut.scheduling.TaskExecutors; import io.micronaut.scheduling.annotation.ExecuteOn; import io.swagger.v3.oas.annotations.Parameter; @@ -119,17 +122,34 @@ public HttpResponse createApp( @Parameter(hidden = true) @NonNull RequestInfo requestInfo) { URI launcherURI = redirectService.getLauncherURI(); try { + String oauthError = requestInfo.getParameters().get("error"); + if (!StringUtils.isEmpty(oauthError)) { + String oauthErrorDescription = requestInfo.getParameters().get("error_description"); + String error = StringUtils.isEmpty(oauthErrorDescription) ? oauthError : oauthErrorDescription; + return handleOAuthError(launcherURI, requestInfo, state, error); + } + + if (StringUtils.isEmpty(code) && StringUtils.isEmpty(state)) { + String oauthState = redirectService.createOAuthState(); + return HttpResponse.temporaryRedirect(redirectService.constructOAuthRedirectUrl(requestInfo, oauthState)) + .cookie(redirectService.createOAuthStateCookie(requestInfo, oauthState, getStoredOAuthStates())); + } + if (StringUtils.isEmpty(code) || StringUtils.isEmpty(state)) { - return HttpResponse.temporaryRedirect(redirectService.constructOAuthRedirectUrl(requestInfo)); + return rejectInvalidOAuthState(launcherURI); + } + + if (!redirectService.isValidOAuthState(state, getStoredOAuthStates())) { + return rejectInvalidOAuthState(launcherURI); + } + + GitHubRepository repository = gitHubCreateService.creatApp( + type, name, features, build, reloading, gorm, servlet, javaVersion, code, state, userAgent); + + if (launcherURI == null) { + return consumeOAuthStateCookie(HttpResponse.ok(new GitHubCreateDTO(repository.getUrl(), repository.getCloneUrl(), repository.getHtmlUrl())), requestInfo, state); } else { - GitHubRepository repository = gitHubCreateService.creatApp( - type, name, features, build, reloading, gorm, servlet, javaVersion, code, state, userAgent); - - if (launcherURI == null) { - return HttpResponse.ok(new GitHubCreateDTO(repository.getUrl(), repository.getCloneUrl(), repository.getHtmlUrl())); - } else { - return HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeRedirectUrl(repository)); - } + return consumeOAuthStateCookie(HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeRedirectUrl(repository)), requestInfo, state); } } catch (Exception e) { if (LOG.isDebugEnabled()) { @@ -139,20 +159,55 @@ public HttpResponse createApp( if (launcherURI == null) { throw e; } else { - return HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeErrorRedirectUrl(e.getMessage())); + return consumeOAuthStateCookie(HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeErrorRedirectUrl(e.getMessage())), requestInfo, state); } } } + private HttpResponse handleOAuthError(URI launcherURI, RequestInfo requestInfo, String state, String error) { + if (!hasValidOAuthState(state)) { + return rejectInvalidOAuthState(launcherURI); + } + if (launcherURI == null) { + return consumeOAuthStateCookie(HttpResponse.badRequest(), requestInfo, state); + } + return consumeOAuthStateCookie(HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeErrorRedirectUrl(error)), requestInfo, state); + } + + private HttpResponse rejectInvalidOAuthState(URI launcherURI) { + if (launcherURI == null) { + return HttpResponse.badRequest(); + } + IllegalArgumentException exception = new IllegalArgumentException("Invalid GitHub OAuth state."); + return HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeErrorRedirectUrl(exception.getMessage())); + } + + private HttpResponse consumeOAuthStateCookie(MutableHttpResponse response, RequestInfo requestInfo, String state) { + return response.cookie(redirectService.consumeOAuthStateCookie(requestInfo, getStoredOAuthStates(), state)); + } + + private String getStoredOAuthStates() { + return ServerRequestContext.currentRequest() + .flatMap(request -> request.getCookies().findCookie(GitHubRedirectService.OAUTH_STATE_COOKIE)) + .map(Cookie::getValue) + .orElse(null); + } + + private boolean hasValidOAuthState(String state) { + return redirectService.isValidOAuthState(state, getStoredOAuthStates()); + } + /** * Endpoint handles GitHub OAuth authorisation errors. * * @param error error code * @param errorDescription description + * @param state The OAuth state + * @param requestInfo The request info * @return Http redirects * @see Troubleshooting OAuth App access token request errors */ - @Get(uri = "/github{?error,error_description}", produces = MediaType.APPLICATION_JSON) + @Get(uri = "/github{?error,error_description,state}", produces = MediaType.APPLICATION_JSON) @ApiResponses(value = { @ApiResponse( responseCode = "307", @@ -164,18 +219,25 @@ public HttpResponse createApp( )}) public HttpResponse handleCallback( @Nullable String error, - @Nullable @QueryValue("error_description") String errorDescription) { + @Nullable @QueryValue("error_description") String errorDescription, + @Nullable @QueryValue("state") String state, + @Parameter(hidden = true) @NonNull RequestInfo requestInfo) { URI redirect; if (!StringUtils.isEmpty(error)) { + if (!hasValidOAuthState(state)) { + return rejectInvalidOAuthState(redirectService.getLauncherURI()); + } redirect = redirectService.constructGrailsForgeErrorRedirectUrl(errorDescription); } else { redirect = redirectService.getLauncherURI(); } if (redirect == null) { - return HttpResponse.ok(errorDescription); + MutableHttpResponse response = HttpResponse.ok(errorDescription); + return hasValidOAuthState(state) ? consumeOAuthStateCookie(response, requestInfo, state) : response; } else { - return HttpResponse.temporaryRedirect(redirect); + MutableHttpResponse response = HttpResponse.temporaryRedirect(redirect); + return hasValidOAuthState(state) ? consumeOAuthStateCookie(response, requestInfo, state) : response; } } } diff --git a/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java b/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java index 388f220e3c4..142266ef32e 100644 --- a/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java +++ b/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java @@ -20,6 +20,8 @@ import io.micronaut.context.annotation.Property; import io.micronaut.context.annotation.Requires; +import io.micronaut.http.cookie.Cookie; +import io.micronaut.http.cookie.SameSite; import io.micronaut.http.uri.UriBuilder; import org.grails.forge.api.RequestInfo; import org.grails.forge.api.GrailsForgeConfiguration; @@ -44,8 +46,12 @@ public class GitHubRedirectService { public static final String OAUTH_URL = "micronaut.http.services." + GitHubOAuthClient.SERVICE_ID + ".url"; + public static final String OAUTH_STATE_COOKIE = "GRAILS_FORGE_GITHUB_OAUTH_STATE"; private static final Logger LOG = LoggerFactory.getLogger(GitHubRedirectService.class); private static final String AUTHORIZE_PATH = "/login/oauth/authorize"; + private static final String OAUTH_STATE_COOKIE_PATH_SUFFIX = "/github"; + private static final String OAUTH_STATE_SEPARATOR = "|"; + private static final long OAUTH_STATE_MAX_AGE_SECONDS = 600; private final String githubOAuthUrl; private final GrailsForgeConfiguration starterConfiguration; @@ -61,12 +67,134 @@ public GitHubRedirectService( } /** - * Creates redirect URI to github oauth auhtorise in order to receive the user access code. + * Creates a state value for GitHub OAuth callback validation. + * + * @return The OAuth state + */ + protected String createOAuthState() { + return UUID.randomUUID().toString(); + } + + /** + * Creates the cookie used to persist GitHub OAuth state for callback validation. + * + * @param requestInfo origin request info + * @param state The OAuth state + * @param storedStates The persisted OAuth states + * @return The OAuth state cookie + */ + protected Cookie createOAuthStateCookie(RequestInfo requestInfo, String state, String storedStates) { + return createOAuthStateCookie(requestInfo, appendOAuthState(storedStates, state)); + } + + private Cookie createOAuthStateCookie(RequestInfo requestInfo, String states) { + return Cookie.of(OAUTH_STATE_COOKIE, states) + .httpOnly(true) + .secure(requestInfo.getServerURL().startsWith("https://")) + .sameSite(SameSite.Lax) + .path(resolveOAuthStateCookiePath(requestInfo)) + .maxAge(OAUTH_STATE_MAX_AGE_SECONDS); + } + + /** + * Creates the cookie used to clear persisted GitHub OAuth state. + * + * @param requestInfo origin request info + * @return The expired OAuth state cookie + */ + protected Cookie expireOAuthStateCookie(RequestInfo requestInfo) { + return Cookie.of(OAUTH_STATE_COOKIE, "") + .httpOnly(true) + .secure(requestInfo.getServerURL().startsWith("https://")) + .sameSite(SameSite.Lax) + .path(resolveOAuthStateCookiePath(requestInfo)) + .maxAge(0); + } + + /** + * Creates the cookie used to remove the completed OAuth state from storage. + * + * @param requestInfo origin request info + * @param storedStates The persisted OAuth states + * @param state The completed OAuth state + * @return The updated OAuth state cookie + */ + protected Cookie consumeOAuthStateCookie(RequestInfo requestInfo, String storedStates, String state) { + String remainingStates = removeOAuthState(storedStates, state); + if (remainingStates.isEmpty()) { + return expireOAuthStateCookie(requestInfo); + } + return createOAuthStateCookie(requestInfo, remainingStates); + } + + private String resolveOAuthStateCookiePath(RequestInfo requestInfo) { + String path = URI.create(requestInfo.getServerURL()).getPath(); + if (path == null || path.isEmpty() || "/".equals(path)) { + return OAUTH_STATE_COOKIE_PATH_SUFFIX; + } + if (path.endsWith("/")) { + return path.substring(0, path.length() - 1) + OAUTH_STATE_COOKIE_PATH_SUFFIX; + } + return path + OAUTH_STATE_COOKIE_PATH_SUFFIX; + } + + /** + * Validates that the callback state matches the persisted OAuth state. + * + * @param state The callback state + * @param storedState The persisted state + * @return Whether the state is valid + */ + protected boolean isValidOAuthState(String state, String storedState) { + return containsOAuthState(storedState, state); + } + + private String appendOAuthState(String storedStates, String state) { + if (storedStates == null || storedStates.isEmpty()) { + return state; + } + if (containsOAuthState(storedStates, state)) { + return storedStates; + } + return storedStates + OAUTH_STATE_SEPARATOR + state; + } + + private String removeOAuthState(String storedStates, String state) { + if (storedStates == null || storedStates.isEmpty() || state == null) { + return ""; + } + StringBuilder remainingStates = new StringBuilder(); + for (String storedState : storedStates.split("\\|")) { + if (!storedState.isEmpty() && !storedState.equals(state)) { + if (remainingStates.length() > 0) { + remainingStates.append(OAUTH_STATE_SEPARATOR); + } + remainingStates.append(storedState); + } + } + return remainingStates.toString(); + } + + private boolean containsOAuthState(String storedStates, String state) { + if (storedStates == null || storedStates.isEmpty() || state == null) { + return false; + } + for (String storedState : storedStates.split("\\|")) { + if (storedState.equals(state)) { + return true; + } + } + return false; + } + + /** + * Creates redirect URI to GitHub OAuth authorize in order to receive the user access code. * * @param requestInfo origin request info - * @return URI to github oauth authorise + * @param state The OAuth state + * @return URI to GitHub OAuth authorize */ - protected URI constructOAuthRedirectUrl(RequestInfo requestInfo) { + protected URI constructOAuthRedirectUrl(RequestInfo requestInfo, String state) { try { UriBuilder uriBuilder = UriBuilder.of(requestInfo.getServerURL()).path(requestInfo.getPath()); requestInfo.getParameters().forEachValue(uriBuilder::queryParam); @@ -76,7 +204,7 @@ protected URI constructOAuthRedirectUrl(RequestInfo requestInfo) { .queryParam("scope", gitHubConfiguration.getTokenPermissions()) .queryParam("client_id", gitHubConfiguration.getClientId()) .queryParam("redirect_uri", redirectUri.toString()) - .queryParam("state", UUID.randomUUID().toString()) + .queryParam("state", state) .build(); } catch (Exception e) { String msg = "Failed to construct redirect URI using request " + requestInfo + " to GiHub OAuth: " + e.getMessage(); diff --git a/grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy b/grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy new file mode 100644 index 00000000000..04ae3c0ef69 --- /dev/null +++ b/grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy @@ -0,0 +1,346 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.grails.forge.api.create.github + +import io.micronaut.context.ApplicationContext +import io.micronaut.http.HttpHeaders +import io.micronaut.http.HttpRequest +import io.micronaut.http.HttpResponse +import io.micronaut.http.client.HttpClient +import io.micronaut.runtime.server.EmbeddedServer +import spock.lang.Specification + +class GitHubOAuthStateSpec extends Specification { + + void "github redirect issues oauth state cookie matching authorize url"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpResponse response = httpClient.toBlocking().exchange(HttpRequest.GET('/github/web/demo')) + + then: + response.status.code == 307 + String location = response.header(HttpHeaders.LOCATION) + String state = queryValue(location, 'state') + state + hasStateCookie(response, state, '/github') + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github redirect scopes oauth state cookie under configured path"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration(['grails.forge.path': '/api'])) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpResponse response = httpClient.toBlocking().exchange(HttpRequest.GET('/github/web/demo')) + + then: + response.status.code == 307 + String location = response.header(HttpHeaders.LOCATION) + String state = queryValue(location, 'state') + state + URLDecoder.decode(queryValue(location, 'redirect_uri'), 'UTF-8').endsWith('/api/github/web/demo') + hasStateCookie(response, state, '/api/github') + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github callback rejects mismatched oauth state before token exchange"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github/web/demo?code=code-from-github&state=returned-state') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=issued-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasNoStateCookie(response) + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github callback rejects missing oauth state cookie before token exchange"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpResponse response = httpClient.toBlocking().exchange(HttpRequest.GET('/github/web/demo?code=code-from-github&state=returned-state')) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasNoStateCookie(response) + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github callback rejects missing state parameter before token exchange"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github/web/demo?code=code-from-github') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=issued-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasNoStateCookie(response) + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github create callback forwards oauth error and consumes returned state"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github/web/demo?error=access_denied&error_description=Denied&state=older-state') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=older-state|newer-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Denied') + !response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasStateCookie(response, 'newer-state', '/github') + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github create callback rejects oauth error without state and preserves oauth state cookie"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github/web/demo?error=access_denied&error_description=Denied') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=issued-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + !response.header(HttpHeaders.LOCATION).contains('/login/oauth/authorize') + hasNoStateCookie(response) + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github create callback rejects oauth error with mismatched state and preserves oauth state cookie"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github/web/demo?error=access_denied&error_description=Denied&state=returned-state') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=issued-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasNoStateCookie(response) + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github callback accepts older and newer outstanding oauth states"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest olderRequest = HttpRequest.GET('/github/web/demo?code=code-from-github&state=older-state') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=older-state|newer-state") + HttpResponse olderResponse = httpClient.toBlocking().exchange(olderRequest) + + then: + olderResponse.status.code == 307 + !olderResponse.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasStateCookie(olderResponse, 'newer-state', '/github') + + when: + HttpRequest newerRequest = HttpRequest.GET('/github/web/demo?code=code-from-github&state=newer-state') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=newer-state") + HttpResponse newerResponse = httpClient.toBlocking().exchange(newerRequest) + + then: + newerResponse.status.code == 307 + !newerResponse.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasExpiredStateCookie(newerResponse, '/github') + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github callback rejects mismatched oauth state without clearing other outstanding states"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github/web/demo?code=code-from-github&state=unknown-state') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=older-state|newer-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasNoStateCookie(response) + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github oauth error callback rejects oauth error without state and preserves oauth state cookie"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github?error=access_denied&error_description=Denied') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=issued-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasNoStateCookie(response) + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github oauth error callback consumes returned state and preserves other outstanding oauth states"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github?error=access_denied&error_description=Denied&state=older-state') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=older-state|newer-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Denied') + hasStateCookie(response, 'newer-state', '/github') + + cleanup: + httpClient.close() + embeddedServer.close() + } + + void "github oauth error callback rejects oauth error without state under configured path"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration(['grails.forge.path': '/api'])) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github?error=access_denied&error_description=Denied') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=issued-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') + hasNoStateCookie(response) + + cleanup: + httpClient.close() + embeddedServer.close() + } + + private static Map configuration(Map overrides = [:]) { + Map configuration = [ + 'micronaut.server.port': -1, + 'micronaut.http.client.follow-redirects': false, + 'micronaut.http.services.github-oauth.url': 'http://localhost:9', + 'micronaut.http.services.github-api-v3.url': 'http://localhost:9', + 'grails.forge.github.clientId': 'clientId', + 'grails.forge.github.clientSecret': 'clientSecret' + ] + configuration.putAll(overrides) + configuration + } + + private static String queryValue(String location, String name) { + new URI(location).query.split('&').find { String pair -> + pair.startsWith("${name}=") + }?.substring(name.length() + 1) + } + + private static boolean hasExpiredStateCookie(HttpResponse response, String path) { + response.headers.getAll(HttpHeaders.SET_COOKIE).any { String cookie -> + cookie.contains("${GitHubRedirectService.OAUTH_STATE_COOKIE}=") && + cookie.contains("Path=${path}") && + cookie.contains('Max-Age=0') && + cookie.contains('SameSite=Lax') + } + } + + private static boolean hasStateCookie(HttpResponse response, String state, String path) { + response.headers.getAll(HttpHeaders.SET_COOKIE).any { String cookie -> + cookie.contains("${GitHubRedirectService.OAUTH_STATE_COOKIE}=${state}") && + cookie.contains("Path=${path}") && + cookie.contains('HTTPOnly') && + cookie.contains('SameSite=Lax') && + cookie.contains('Max-Age=600') + } + } + + private static boolean hasNoStateCookie(HttpResponse response) { + !response.headers.getAll(HttpHeaders.SET_COOKIE).any { String cookie -> + cookie.contains("${GitHubRedirectService.OAUTH_STATE_COOKIE}=") + } + } +} From 699b6eb30a2d1299a3d5e141b7de1584c075eb69 Mon Sep 17 00:00:00 2001 From: James Fredley Date: Thu, 9 Jul 2026 15:34:51 -0400 Subject: [PATCH 2/2] Address GitHub OAuth state review feedback Assisted-by: opencode:gpt-5.5 --- .../create/github/GitHubCreateController.java | 48 +---------- .../create/github/GitHubRedirectService.java | 2 +- .../create/github/GitHubOAuthStateSpec.groovy | 80 +++++-------------- 3 files changed, 23 insertions(+), 107 deletions(-) diff --git a/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java b/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java index d3e179bf856..4af48e2dc3d 100644 --- a/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java +++ b/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubCreateController.java @@ -29,7 +29,6 @@ import io.micronaut.http.annotation.Controller; import io.micronaut.http.annotation.Get; import io.micronaut.http.annotation.Header; -import io.micronaut.http.annotation.QueryValue; import io.micronaut.http.context.ServerRequestContext; import io.micronaut.http.cookie.Cookie; import io.micronaut.scheduling.TaskExecutors; @@ -159,7 +158,8 @@ public HttpResponse createApp( if (launcherURI == null) { throw e; } else { - return consumeOAuthStateCookie(HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeErrorRedirectUrl(e.getMessage())), requestInfo, state); + MutableHttpResponse response = HttpResponse.temporaryRedirect(redirectService.constructGrailsForgeErrorRedirectUrl(e.getMessage())); + return hasValidOAuthState(state) ? consumeOAuthStateCookie(response, requestInfo, state) : response; } } } @@ -196,48 +196,4 @@ private String getStoredOAuthStates() { private boolean hasValidOAuthState(String state) { return redirectService.isValidOAuthState(state, getStoredOAuthStates()); } - - /** - * Endpoint handles GitHub OAuth authorisation errors. - * - * @param error error code - * @param errorDescription description - * @param state The OAuth state - * @param requestInfo The request info - * @return Http redirects - * @see Troubleshooting OAuth App access token request errors - */ - @Get(uri = "/github{?error,error_description,state}", produces = MediaType.APPLICATION_JSON) - @ApiResponses(value = { - @ApiResponse( - responseCode = "307", - description = "Forwarded GitHub OAuth error message." - ), - @ApiResponse( - responseCode = "200", - description = "Returns GitHub OAuth application callback error." - )}) - public HttpResponse handleCallback( - @Nullable String error, - @Nullable @QueryValue("error_description") String errorDescription, - @Nullable @QueryValue("state") String state, - @Parameter(hidden = true) @NonNull RequestInfo requestInfo) { - URI redirect; - if (!StringUtils.isEmpty(error)) { - if (!hasValidOAuthState(state)) { - return rejectInvalidOAuthState(redirectService.getLauncherURI()); - } - redirect = redirectService.constructGrailsForgeErrorRedirectUrl(errorDescription); - } else { - redirect = redirectService.getLauncherURI(); - } - - if (redirect == null) { - MutableHttpResponse response = HttpResponse.ok(errorDescription); - return hasValidOAuthState(state) ? consumeOAuthStateCookie(response, requestInfo, state) : response; - } else { - MutableHttpResponse response = HttpResponse.temporaryRedirect(redirect); - return hasValidOAuthState(state) ? consumeOAuthStateCookie(response, requestInfo, state) : response; - } - } } diff --git a/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java b/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java index 142266ef32e..3853e3c7587 100644 --- a/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java +++ b/grails-forge/grails-forge-api/src/main/java/org/grails/forge/api/create/github/GitHubRedirectService.java @@ -207,7 +207,7 @@ protected URI constructOAuthRedirectUrl(RequestInfo requestInfo, String state) { .queryParam("state", state) .build(); } catch (Exception e) { - String msg = "Failed to construct redirect URI using request " + requestInfo + " to GiHub OAuth: " + e.getMessage(); + String msg = "Failed to construct redirect URI using request " + requestInfo + " to GitHub OAuth: " + e.getMessage(); LOG.error(msg, e); throw new RuntimeException(msg); } diff --git a/grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy b/grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy index 04ae3c0ef69..1ebaa023c11 100644 --- a/grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy +++ b/grails-forge/grails-forge-api/src/test/groovy/org/grails/forge/api/create/github/GitHubOAuthStateSpec.groovy @@ -149,6 +149,26 @@ class GitHubOAuthStateSpec extends Specification { embeddedServer.close() } + void "github create callback falls back to oauth error code when description is missing"() { + given: + EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) + HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) + + when: + HttpRequest request = HttpRequest.GET('/github/web/demo?error=access_denied&state=older-state') + .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=older-state|newer-state") + HttpResponse response = httpClient.toBlocking().exchange(request) + + then: + response.status.code == 307 + response.header(HttpHeaders.LOCATION).contains('access_denied') + hasStateCookie(response, 'newer-state', '/github') + + cleanup: + httpClient.close() + embeddedServer.close() + } + void "github create callback rejects oauth error without state and preserves oauth state cookie"() { given: EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) @@ -240,66 +260,6 @@ class GitHubOAuthStateSpec extends Specification { embeddedServer.close() } - void "github oauth error callback rejects oauth error without state and preserves oauth state cookie"() { - given: - EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) - HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) - - when: - HttpRequest request = HttpRequest.GET('/github?error=access_denied&error_description=Denied') - .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=issued-state") - HttpResponse response = httpClient.toBlocking().exchange(request) - - then: - response.status.code == 307 - response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') - hasNoStateCookie(response) - - cleanup: - httpClient.close() - embeddedServer.close() - } - - void "github oauth error callback consumes returned state and preserves other outstanding oauth states"() { - given: - EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration()) - HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) - - when: - HttpRequest request = HttpRequest.GET('/github?error=access_denied&error_description=Denied&state=older-state') - .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=older-state|newer-state") - HttpResponse response = httpClient.toBlocking().exchange(request) - - then: - response.status.code == 307 - response.header(HttpHeaders.LOCATION).contains('Denied') - hasStateCookie(response, 'newer-state', '/github') - - cleanup: - httpClient.close() - embeddedServer.close() - } - - void "github oauth error callback rejects oauth error without state under configured path"() { - given: - EmbeddedServer embeddedServer = ApplicationContext.run(EmbeddedServer, configuration(['grails.forge.path': '/api'])) - HttpClient httpClient = embeddedServer.applicationContext.createBean(HttpClient, embeddedServer.URL) - - when: - HttpRequest request = HttpRequest.GET('/github?error=access_denied&error_description=Denied') - .header(HttpHeaders.COOKIE, "${GitHubRedirectService.OAUTH_STATE_COOKIE}=issued-state") - HttpResponse response = httpClient.toBlocking().exchange(request) - - then: - response.status.code == 307 - response.header(HttpHeaders.LOCATION).contains('Invalid+GitHub+OAuth+state') - hasNoStateCookie(response) - - cleanup: - httpClient.close() - embeddedServer.close() - } - private static Map configuration(Map overrides = [:]) { Map configuration = [ 'micronaut.server.port': -1,