Repository navigation
feat(appcheck): Verify one-time tokens for replay protection - #774
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the VerifyOneTimeToken method to the App Check client, enabling stateful verification and consumption of one-time tokens. It also adds the AlreadyConsumed field to DecodedAppCheckToken and integrates HTTP client options. Feedback on the tests suggests using a helper function to instantiate *bool pointers to reduce verbosity, and restoring the mutated global verifyURLFormat variable to prevent potential race conditions and side effects.
weixifan
left a comment
There was a problem hiding this comment.
Thank you for the PR; LGTM with just a minor comment.
| const appCheckIssuer = "https://firebaseappcheck.googleapis.com/" | ||
|
|
||
| var ( | ||
| verifyURLFormat = "https://firebaseappcheck.googleapis.com/v1beta/projects/%s:verifyAppCheckToken" |
There was a problem hiding this comment.
Our v1 endpoint is almost ready, and that would be our preferred URL when this SDK is released. Maybe we can just change this to v1 now?
I'm also fine with leaving this alone for now and wait until our v1 endpoint is fully published. In that case, is a TODO appropriate here?
There was a problem hiding this comment.
Thanks for the quick review! Sure, we can switch to the v1 endpoint as long as we can verify and run our tests against it. Currently, we only have unit tests in place so we can just change to v1 now, but we plan to add integration tests against the live endpoint once we implement the token creation methods.
There was a problem hiding this comment.
One thing we should keep in mind if we do merge this as v1, is that our next release will include this whether or not the backend is ready. Lets set v1 but hold back merging until the backend is live.
jonathanedey
left a comment
There was a problem hiding this comment.
LGTM with one note on v1 endpoint. Thanks!
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
nit because I am unsure if we track this.
This is missing the usage headers
hc.Opts = []internal.HTTPOption{
internal.WithHeader("x-goog-api-client", internal.GetMetricsHeader(config.Version)),
}| // | ||
| // This method performs the same stateless verification as VerifyToken. In addition, it makes a | ||
| // stateful network call to the Firebase App Check backend to ensure that the token has not been | ||
| // consumed previously. If the token is valid, it is marked as consumed. |
There was a problem hiding this comment.
I wonder if we can add a few things in here. It would be nice to make it clear that this method doesn't throw an error for already consumed tokens. Maybe we can incorporate some wordings from the Node.js docs?
/**
* To use the replay protection feature, set this to `true`. The {@link AppCheck.verifyToken}
* method will mark the token as consumed after verifying it.
*
* Tokens that are found to be already consumed will be marked as such in the response.
*
* Tokens are only considered to be consumed if it is sent to App Check backend by calling the
* {@link AppCheck.verifyToken} method with this field set to `true`; other uses of the token
* do not consume it.
*
* This replay protection feature requires an additional network call to the App Check backend
* and forces your clients to obtain a fresh attestation from your chosen attestation providers.
* This can therefore negatively impact performance and can potentially deplete your attestation
* providers' quotas faster. We recommend that you use this feature only for protecting
* low volume, security critical, or expensive operations.
*/
| jwks, err := keyfunc.Get(JWKSUrl, keyfunc.Options{ | ||
| Ctx: ctx, | ||
| RefreshInterval: 6 * time.Hour, | ||
| }) |
There was a problem hiding this comment.
Should we add a check for ProjectID here?
if c.ProjectID == "" {
return nil, errors.New("project ID is required to access Firebase App Check client")
}|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for verifying and consuming one-time App Check tokens by introducing the VerifyOneTimeToken method, which enables replay protection. It also updates the JWKS URL to the v1 endpoint, adds project ID validation in NewClient, and configures an internal HTTP client with metrics headers. The review feedback suggests adding a nil check for the configuration parameter in NewClient to prevent potential nil pointer dereferences, and appending to hc.Opts instead of overwriting it to preserve existing client options.
| if conf.ProjectID == "" { | ||
| return nil, errors.New("project ID is required to access Firebase App Check client") | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
NewClient is only called internally from App.AppCheck, which always passes a non-nil config, and internal.AppCheckConfig can't be constructed outside the module. This also matches the other service constructors, which don't nil-check their config either, so I'd prefer to keep it consistent.
| hc.Opts = []internal.HTTPOption{ | ||
| internal.WithHeader("x-goog-api-client", internal.GetMetricsHeader(conf.Version)), | ||
| } |
There was a problem hiding this comment.
hc is freshly created by internal.NewHTTPClient on the line above, which never populates Opts, so there's nothing to preserve and both forms behave identically.
This PR introduces the
VerifyOneTimeTokenmethod to theappcheckpackage, enabling stateful replay protection for Firebase App Check.VerifyOneTimeTokenthat first performs local, stateless JWT verification followed by a statefulPOSTrequest to backend endpoint to consume the token.AlreadyConsumed(*bool) field to theDecodedAppCheckTokenstruct.option.ClientOptionfrom the main app initialization down tointernal.AppCheckConfig.