PresentationDefinition fetches the remote PD endpoint twice per request

Open Beginner friendly
#4,435 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
82/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Quiet
Tech stack
go
Domain
api, backend

Research direction

Start in auth/client/iam/client.go at HTTPClient.PresentationDefinition and inspect the existing tests for the IAM client. Add a focused test that counts requests to the remote presentation-definition endpoint, then run the relevant Go tests. Done means one GET per resolution, the populated presentation definition is returned, and successful requests produce no error.

Written by the indexing model from the issue text.

Description

auth bug

Summary

HTTPClient.PresentationDefinition issues the same GET request twice. Every presentation definition resolution therefore makes two identical calls to the remote authorization server's PD endpoint.

Where

auth/client/iam/client.go:100-118:

func (hb HTTPClient) PresentationDefinition(ctx context.Context, presentationDefinitionURL url.URL) (*pe.PresentationDefinition, error) {
	// create a GET request with scope query param
	request, err := http.NewRequestWithContext(ctx, http.MethodGet, presentationDefinitionURL.String(), nil)
	if err != nil {
		return nil, err
	}
	var presentationDefinition pe.PresentationDefinition
	err = hb.doRequest(ctx, request, &presentationDefinition)
	if err != nil {
		// any OAuth error should be passed
		// any other error should result in a 502 Bad Gateway
		if errors.As(err, new(oauth.OAuth2Error)) {
			return nil, err
		}
		return nil, errors.Join(ErrBadGateway, err)
	}

	return &presentationDefinition, hb.doRequest(ctx, request, &presentationDefinition)
}

The final line calls doRequest a second time, using it as the returned error expression. The first call has already fetched and unmarshalled the document, and its error has already been handled above.

Because the request is a GET with a nil body it is safely reusable, so both calls succeed and the duplication is invisible in normal operation.

Impact

Two consequences, neither security-relevant.

Outbound load on every peer's presentation definition endpoint is doubled. The call path is PresentationDefinitionResolver.resolveRemote (auth/client/iam/pd_resolver.go:64-79), reached from requestVPTokenAccessToken (auth/client/iam/openid4vp.go:302), so it runs on every RFC021 service access token request that resolves its PD remotely.

The returned error is the second call's error rather than the first's. A transient failure on the redundant request makes the function return a non-nil error alongside a valid *pe.PresentationDefinition. Callers check the error first, so a presentation definition that was fetched successfully gets discarded and the token request fails for no real reason.

Fix

Return the already-populated value:

	return &presentationDefinition, nil

A test asserting that a single call reaches the endpoint once would prevent a recurrence. The existing tests pass with the duplicate because both requests succeed.

Dominant language
Go
Stars
28
Forks
23
Avg merge
1d 10h
Merged PRs (30d)
76

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from nuts-foundation/nuts-node

All issues in nuts-foundation/nuts-node

Similar issues

More Go issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.