michael-lazar / gemini-diagnostics

A torture test for gemini servers
MIT License
25 stars 5 forks source link

Don't error when <META> is empty #1

Closed ento closed 3 years ago

ento commented 3 years ago

If my reading is correct, the spec (v0.14.2) doesn't always require the presence of the <META> string in the header:

3.2.4 4x (TEMPORARY FAILURE)
[..] The contents of \ may provide additional information on the failure, and should be displayed to human users. 3.2.5 5x (PERMANENT FAILURE)
[..] The contents of \ may provide additional information on the failure, and should be displayed to human users. [..]

(emphasis mine)

This PR makes it so that the script is able to parse header when it only consists of the status code.

Given this simple Node.js server that always returns 59 ```js const tls = require('tls'); const fs = require('fs'); const path = require('path'); const options = { key: fs.readFileSync(path.join(__dirname, './tls.key')), cert: fs.readFileSync(path.join(__dirname, './tls.crt')), }; const server = tls.createServer(options, (socket) => { socket.end('59\r\n'); }); server.listen(1965, () => { console.log('listening'); }); ```

Before

$ ./gemini-diagnostics --checks URLEmpty
Running server diagnostics check against localhost:1965
...

[URLEmpty] Empty URLs should not be accepted by the server
Request URL
  '\r\n'
Response header
  '59\r\n'
  x not enough values to unpack (expected 2, got 1)

Done!

After

$ ./gemini-diagnostics --checks URLEmpty
Running server diagnostics check against localhost:1965
...

[URLEmpty] Empty URLs should not be accepted by the server
Request URL
  '\r\n'
Response header
  '59\r\n'
Status should return a failure code (59 BAD REQUEST)
  ✓ Received status of '59'

Done!
michael-lazar commented 3 years ago

Hello! While I agree that the spec is unclear in this regard, I believe the intention of gemini is that response meta should never be empty. Even solderpunk's own client makes this assertion:

https://tildegit.org/solderpunk/AV-98/src/branch/master/av98.py#L430

        # Validate header
        status, meta = header.split(maxsplit=1)
        if len(meta) > 1024 or len(status) != 2 or not status.isnumeric():
            f.close()
            raise RuntimeError("Received invalid header from server!")

If the meta was truly optional, I would expect the specification to be written as

<STATUS>[<SPACE><META>]<CR><LF>

using brackets to denote optional parts, per ABNF syntax

ento commented 3 years ago

I see - I've pushed a commit that makes checks fail when <META> is missing. I hope this improves the descriptiveness of the error message.

$ ./gemini-diagnostics --checks URLInvalid
Running server diagnostics check against localhost:1965
...

[URLInvalid] Random text should not be accepted by the server
Request URL
  'Hello Gemini!\r\n'
Response header
  '59\r\n'
  x Status should include a <META> line

Done!
michael-lazar commented 3 years ago

Thanks a lot! If it turns out that I was wrong on my interpretation, I will be happy to revert to your previous version of the PR too.

ento commented 3 years ago

Thanks for the merge!