sync up various TODOs with issues #2417
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- where it's not particularly valuable to have a TODO in the code that corresponds to something, and there's now an issue, I removed the comment altogether - when I felt it was still useful to have a comment (e.g., because a reader might want to know about the problem, or because it's useful to point out exactly where some problem exists), I left the comment but updated it to reference the issue
davepacheco
commented
Feb 23, 2023
| /// can fail (if the value is larger than i64::MAX). We provide all of these for | ||
| /// consumers' convenience. | ||
| // TODO-cleanup This could benefit from a more complete implementation. | ||
| // TODO-correctness RFD 4 requires that this be a multiple of 256 MiB. We'll |
| ); | ||
|
|
||
| -- Access tokens granted in response to successful device authorization flows. | ||
| -- TODO-security: expire tokens. |
| /// suggested in RFC 8628 §6.1 (User Code Recommendations); q.v. also for | ||
| /// a discussion of entropy requirements. On input, use codes should be | ||
| /// uppercased, and characters not in this alphabet should be stripped. | ||
| // TODO-security: user code tries should be rate-limited |
| message: "device authorization request expired".to_string(), | ||
| }) | ||
| } else { | ||
| // TODO-security: set an expiration time for the valid token. |
Comment on lines
-8
to
-14
| // - TCP and HTTP KeepAlive parameters | ||
| // - Server hostname | ||
| // - Disable signals? | ||
| // - Analogs for actix client_timeout (request timeout), client_shutdown (client | ||
| // shutdown timeout), server backlog, number of workers, max connections per | ||
| // worker, max connect-in-progress sockets, shutdown_timeout (server shutdown | ||
| // timeout) |
Collaborator
Author
| where | ||
| T: authz::ApiResourceWithRolesType + AuthorizedResource + Clone, | ||
| { | ||
| // TODO-security We should carefully review what permissions are |
| } | ||
|
|
||
| /// Successful access token grant. See RFC 6749 §5.1. | ||
| /// TODO-security: `expires_in`, `refresh_token`, etc. |
Collaborator
Author
This was referenced Feb 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I recently used todos to audit the various TODO comments I've written to make sure we had issues filed for anything that should be tracked for MVP. In this PR, for issues that I filed as part of doing this, I'm updating the corresponding TODO comments. There are basically two cases: where the comment was now unnecessary, I just removed it. Where the comment still seemed useful to readers or because the spot where the comment lives is important, I updated it to reference the issue.
In the second commit, I also went and normalized a few various TODO tags to fix misspellings (e.g., TODO-completess).