Skip to content

Week of Oct 6#412

Merged
duglin merged 5 commits into
xregistry:mainfrom
duglin:Oct6
Oct 9, 2025
Merged

Week of Oct 6#412
duglin merged 5 commits into
xregistry:mainfrom
duglin:Oct6

Conversation

@duglin

@duglin duglin commented Oct 3, 2025

Copy link
Copy Markdown
Contributor
  • typos
  • Added support for OPTIONS HTTP method
  • Make "Method not supported" error less http specific

duglin added 2 commits October 3, 2025 12:33
- typos

Signed-off-by: Doug Davis <duglin@gmail.com>
Signed-off-by: Doug Davis <duglin@gmail.com>
Comment thread core/http.md

#### HTTP OPTIONS Method

A server MAY support clients querying the list of supported HTTP methods

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this usually supported out of the box by simple "just dump a load of files" hosting systems? (That being one of the design goals...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All APIs are optional based on need and ability to support them in the hosting environment. I think OPTIONS is no different than POST - neither of which would probably be supported if the registry is hosted on S3.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think I'd misread this as MUST or SHOULD for some reason...

Comment thread core/spec.md Outdated
* Code: `405 Method Not Allowed`
* Instance: `<REQUEST URL>`
* Title: `The specified HTTP method (<INVALID METHOD>) is not supported for: <REQUEST URL>`
* Title: `The specified action (<INVALID ACTiON>) is not supported for: <REQUEST URL>`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
* Title: `The specified action (<INVALID ACTiON>) is not supported for: <REQUEST URL>`
* Title: `The specified action (<INVALID ACTION>) is not supported for: <REQUEST URL>`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done - nice catch

Comment thread core/spec.md Outdated
* Detail: `<THE INVALID DATA>`

### method_not_allowed
### action_not_allowed

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmmm.... this name feels like it invites ambiguity. If I'm "not allowed" to do something, is it because I don't have permission to do it, or because what I'm asking is inherently invalid? It sounds like in this case, it's really about the latter - but I suspect it could easily be interpreted as the former.

For HTTP, code 405 is (I believe, anyway) specific about what's supported by the resource, as opposed to permissions. But if we're trying to describe it generically, perhaps we should call this "action_not_suppported"? After all, that's what the title description mentions...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Couple of things...

  • "not allowed" is taken directly from the http spec so I'm leaning on the fact folks seem to be ok with them using that phrase
  • I think the HTTP is purposely vague as to whether it's "no permission" or "inherently invalid" (no support at all) because to the end user it's the same net result - they can't do it. And I like that because from a security perspective I don't think we want hackers to know the difference. Telling a hacker they need "more permission" is an invite to keep trying, or provides insight into something they're not allowed to access.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm... if an HTTP server returned 405 to me and I could fix it by authenticating, I'd be very surprised that it hadn't returned a 403 instead. Maybe that's just me - might be worth discussing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, but I think that's an impl choice. And our spec even talks about how people can choose a more appropriate error message for their needs.

In general I do agree that 405 is probably more likely to mean the method isn't supported at all regardless of auth. However, as with most HTTP error code, there could be any number of reasons for why the error was generated and I think "not allowed" is vague/wide enough to cover lots of them.

Who would have thought that s/method/action/ would be the focus of the review! :-) I just wanted to remove the HTTP specific aspect ("method") from the error. If my use of the word "supported" in the text causes you concern I can twiddle that. But I do like the synergy with HTTP's 405 status code's phrase.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just did: s/supported/allowed/ in the text for consistency. :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Use: action_not_supported

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

LGTM

Comment thread core/http.md Outdated

If an HTTP method is not supported for a supported HTTP path, then an error
([method_not_allowed](./spec.md#method_not_allowed)) MUST be generated.
([action](./spec.md#action_not_allowed)) MUST be generated.

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.

Suggested change
([action](./spec.md#action_not_allowed)) MUST be generated.
([action_not_allowed](./spec.md#action_not_allowed)) MUST be generated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Signed-off-by: Doug Davis <duglin@gmail.com>
duglin added 2 commits October 9, 2025 15:09
Signed-off-by: Doug Davis <duglin@gmail.com>
Signed-off-by: Doug Davis <duglin@gmail.com>
@duglin

duglin commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

Approved on the 10/9 call

@duglin
duglin merged commit fe4d20d into xregistry:main Oct 9, 2025
2 checks passed
@duglin
duglin deleted the Oct6 branch October 9, 2025 19:42
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.

4 participants