fiatjaf
blossom: redirect support
feat(blossom): add redirect support for GET requests
As far as I can tell these changes are backwards compatible since the new events are optional and the default redirect implementation is hidden behind a new option (variadic Function configuration).
This is just a first draft, but tests with both Cloudflare R2 and IPFS are looking promising so far.
I like the string template thing, but I don't think we should have it, or maybe it should be a helper inside khatru/policies. Also the variadic argument option doesn't exist for all the other functions so it feels weird to have it just for the redirect. I think it's better to just have it as a normal hook like the others.
But I think the core functionality is good, right? Can I merge this and rearrange things around myself?
Sure, go ahead, it's all yours to do as you please. And if you want me to change anything, just let me know as well. The variadic options idea came up as a mix of "I don't want to break backwards compatibility" and "Redirect functionality may evolve beyond GET". For example, we may want to redirect OPTIONS, HEAD, etc. Having a single option at the BlossomServer level makes something like this possible (think go-nostr Pool Options). However, this is completely separate from the core redirect functionality and can easily be removed entirely or converted into one of the mentioned ptions. Another alternative would be to simply expose redirectGet publicly and give it a more descriptive name (say BlossomRedirectPolicy) if you'd prefer to keep it. I think of it as a default implementation of an interface, just expressed in a functional way.
I see, thanks, but, guess what, after using them for years I've finally decided that go-nostr variadic options are horrible. I don't know who had this idea of variadic option but that person is a criminal and I should never have copied it. I will never use that again and hopefully someday that will also be removed from go-nostr.
I don't disagree here 🤣. Lots of mental gymnastics not to include default arguments in Go. Unfortunately this one is out of my league to fix. Someone has to convince Go language designers that "simple" can mean a lot of things. Not having default parameters sucks for backwards compatibility. Feel free to remove the functionality or introduce it in any other way. Unrelated: Speaking with hzdr, I think it may be worth adding a warning / information box to the docs asking users to keep the hash intact as part of the redirect. Are you OK with me adding this?
Yes, please.
But what are the requirements for the redirect? The hash must be present "somewhere"? We can do a string match on the result to check that on the khatru side too and error otherwise. Maybe show an error image so everybody sees it when embedding an image from a broken blossom server.
Hey fiatjaf. Sorry for the long hiatus. Will work on this tomorrow. At the moment, there isn't much in the specification. It basically requires the redirected URL to contain the SHA256:
If the endpoints returns a 301 or 302 redirect it MUST redirect to a URL containing the same sha256 hash as the requested blob.This ensures that if a user was to copy or reuse the redirect URL it would still contain the original sha256 hash
Source: https://github.com/hzrd149/blossom/blob/master/buds/01.md#get-sha256---get-blob
I can add a check to make sure that there's a sequence of 64 hex characters somewhere in the string but it is honestly a bit useless as 64 hex like characters in the URL doesn't tell us much. IMO, if the specification doesn't change, a nice VitePress custom container (https://vitepress.dev/guide/markdown#custom-containers) in yellow or red will do more than a runtime error here (we can have both anyway).
There's also an ongoing discussion about making file extensions mandatory (see https://github.com/hzrd149/blossom/issues/69#issuecomment-2876952621).
I'm planning to do some fine-tuning on this part of the specifications anyway, as I want to allow Blossom servers to redirect HEAD and OPTIONS requests as well (and for this, we need to allow codes 307/308). If we achieve consensus and @hzrd149 agrees to make extensions mandatory on redirect, then the runtime error becomes a bit more useful as we can check for the presence of "{sha256}.{ext}" instead of just "{sha256}". It also simplifies the redirect logic a bit. But this isn't a blocker for this PR; we can introduce it later.
OK, that looks good. I'll wait for you to update here and merge this. After merging I will look at the code.