Enforce that the HireFire token is present in the Flask HireFire endpoint.

[owenniles]

Per the HireFire docs (e.g. this page), The HireFire info endpoint should be of the form /hirefire/{HIREFIRE_TOKEN}/info.

Currently, hirefire.contrib.flask.blueprint.build_hirefire_blueprint registers a URL route that matches not just /hirefire/{HIREFIRE_TOKEN}/info, but any URL path of the form /hirefire/{...}/info. This could be considered a security vulnerability because it is possible to access the HireFire info endpoint without knowing the HireFire token. Practically, however, I think this is just a bug that causes the implementation to be slightly out of sync with the specification.

The changes in this PR enforce that the info endpoint's path is specifically /hirefire/{HIREFIRE_TOKEN}/info.

[codecov]

Codecov Report

Merging #61 (1d321ed) into main (665217e) will increase coverage by 3.12%.
The diff coverage is 100.00%.

❗ Current head 1d321ed differs from pull request most recent head 5957ccc. Consider uploading reports for the commit 5957ccc to get more accurate results

@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
+ Coverage   55.55%   58.68%   +3.12%     
==========================================
  Files          11       11              
  Lines         423      426       +3     
==========================================
+ Hits          235      250      +15     
+ Misses        188      176      -12     

Impacted Files	Coverage Δ
hirefire/contrib/flask/blueprint.py	88.23% <100.00%> (+88.23%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 665217e...5957ccc. Read the comment docs.

[ryanhiebert]

Thanks for submitting. Can you help clarify what this does? It is fixing a bug, or patching a security issue?

[owenniles]

This PR patches what I would consider to be a low-priority security issue. I've just made a few updates to the PR description.

[ryanhiebert]

OK, thank you, that was helpful explanation, and I agree that this is a minor security concern.

In Django, I would be hesitant to use an implementation like this, to hard-code the token in the url path. This is because in some scenarios, the available url routes might be exposed. I think in particular of DEBUG mode, where it'll list out the urls for you. Now, DEBUG mode is plenty dangerous in other ways to make it exceedingly unsafe to run in production ever, but it leads me to think that it might be better to make a runtime check of the token, rather than to hard-code the token in the url path.

What do you think? Are there similar risks with Flask? What do you think about continuing to have it as a parameter, but actually check to make sure it is the right one before getting the data?

[ryanhiebert]

Can you tell me why you chose to hard code the token into the path, rather than checking the token? Do you think my concern is reasonable?

[owenniles]

Sorry for the late response. I think your concern makes a lot of sense and I'm glad you picked up on that because I definitely wouldn't have. I'm having a bit of a busy week, but I will address this concern by modifying my patch on Saturday.