Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixed wkd.lookup in case of HTML being returned in fetch response (closes #3) #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

titanism
Copy link

@titanism titanism commented Feb 6, 2024

This fixes wkd.lookup as per #3.

@twiss
Copy link
Member

twiss commented Aug 13, 2024

Hi 👋 Apologies for the really long delay. I'm not sure this is really needed, as presumably the HTML response will naturally fail to parse as an OpenPGP key, and you can just report that error instead?

Also, draft-koch-openpgp-webkey-service-18 says:

The server SHOULD use "application/octet-stream" as the Content-Type for the data but clients SHOULD also accept any other Content-Type.

So, according to that text we shouldn't reject OpenPGP keys served with a text/html MIME type (even though it's of course technically incorrect).

@titanism
Copy link
Author

@twiss we just updated our PR to remove the Content-Type headers check. The reason for returning early if HTML content is detected is to prevent unnecessary CPU cycles/memory usage.

@twiss
Copy link
Member

twiss commented Aug 13, 2024

I don't think you're really decreasing CPU cycles here, because the check that openpgp.readKey does is really simple; basically it's (bytes[0] & 0x80) !== 0. Much cheaper than decoding the bytes as UTF-8, and then running a regex to check if it's HTML.

Furthermore, in the success case (where the response is a valid OpenPGP key), you're increasing CPU cycles for no reason - and that's where the performance matters the most, IMHO.
If you want to show a more friendly error message, you could always check if it's HTML after openpgp.readKey throws an error (but outside this function), so that it only impacts the error case.

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