Skip to content

Conversation

@vsbuffalo
Copy link

Thank you again for oras-py! I have found a few more issues and have included a
PR below. I will also try to submit a PR soon for #217 (I was able to address
that downstream so less pressing, but I will submit PR soon).

Problems Fixed

1. Authentication headers not being applied consistently

The do_request() method only adds authentication headers when very specific conditions are met:

if isinstance(self.auth, oras.auth.TokenAuth) and self.auth.token is not None:
    headers.update(self.auth.get_auth_header())

This restrictive check causes authentication to fail in several scenarios:

  • When using custom auth backends that aren't exactly TokenAuth instances
  • When auth backends are wrapped or proxied
  • When using OAuth2 bearer tokens with cloud registries (Docker Hub, ECR, GCR, ACR, Artifact Registry, etc.)

2. JSONDecodeError on non-JSON registry responses

When registries return non-JSON responses (e.g., HTML error pages from web portals), the code attempts to parse them with r.json(), resulting in confusing errors:

json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

This commonly occurs with any cloud registry when:

  • Authentication failures return HTML error pages instead of JSON
  • Hitting web portals/management consoles instead of registry API endpoints
  • Network issues or misconfigurations redirect to error pages

Solutions

Commit 1: Use duck typing for auth header application

Relaxes the authentication check to use duck typing, which is more Pythonic and flexible:

if self.auth and hasattr(self.auth, 'get_auth_header'):
    auth_headers = self.auth.get_auth_header()
    if auth_headers:  # Only update if we got actual headers
        headers.update(auth_headers)

Note: In the future, defining an auth interface with a Python Protocol could make the contract more explicit, but the current duck typing approach works well and is less opinionated of a design choice.

Commit 2: Improve error handling for non-JSON responses

  • Replace r.json() calls in error handling with descriptive HTTP status messages
  • Add fallback for capitalized Location header (some registries use "Location" instead of "location")
  • Provide actionable error messages for debugging authentication issues

Benefits

  1. Duck typing: More Pythonic - if an object has get_auth_header(), we can use it
  2. Works with all auth backends: Not limited to just TokenAuth class
  3. Better error messages: Users see actual problems (auth, permissions, connectivity) instead of JSON parsing errors
  4. Robust header handling: Works with registries that use different header capitalizations
  5. Backward compatible: All existing auth backends already implement required methods

Testing

All unit tests pass. I have tested with OAuth2 bearer token authentication (Azure Container Registry):

  • Before: Authentication headers not consistently applied, push operations fail with confusing JSON decode errors
  • After: Headers properly added to all requests, clear error messages, push operations succeed

Verified with cloud registry implementations including Azure Container Registry.

Compatibility

Both changes are fully backward compatible:

  • All existing auth backends (BasicAuth, TokenAuth) already have get_auth_header() method
  • Error handling improvements only change error message content, not behavior
  • The changes only extend support to additional auth backend implementations and registry types

  Changes isinstance check to hasattr for more flexible auth backend support.
  This allows authentication headers to be properly applied for all auth
  backends that implement get_auth_header(), not just TokenAuth instances.

  Fixes authentication issues with OAuth2 tokens and custom auth backends.
When registry returns non-JSON responses (e.g., HTML error pages),
attempting to parse with r.json() causes JSONDecodeError. This commonly
occurs with authentication failures or when hitting web portals instead
of registry APIs.

Changes:
- Replace r.json() calls with descriptive HTTP status messages
- Add fallback for capitalized Location header (some registries use "Location")
- Provide actionable error messages for debugging authentication issues

This prevents confusing "Expecting value: line 1 column 1 (char 0)" errors
and helps users understand the actual problem (auth, permissions, connectivity).
Copy link
Contributor

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

Very thorough, and is reasonable and overall LGTM. Could you please update the CHANGELOG and bump the version?

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