Skip to content

chore: Add linter rule to catch missing return after http writes #2702

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 28, 2022
Merged

chore: Add linter rule to catch missing return after http writes #2702

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
227e1c5
chore: Add linter rule to catch missing return after http writes
Emyrk Jun 28, 2022
aec49c5
Catch broader set of conditions in the if statement
Emyrk Jun 28, 2022
56885dd
Update linter message
Emyrk Jun 28, 2022
120309b
Merge remote-tracking branch 'origin/main' into stevenmasley/http_wri…
Emyrk Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions coderd/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
Message: "Internal error.",
})
return
}
if !equal {
// This message is the same as above to remove ease in detecting whether
Expand Down
22 changes: 22 additions & 0 deletions scripts/rules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,28 @@ func HttpAPIErrorMessage(m dsl.Matcher) {
Report("Field \"Message\" should be a proper sentence with a capitalized first letter and ending in punctuation. $m")
}

// HttpAPIReturn will report a linter violation if the http function is not
// returned after writing a response to the client.
func HttpAPIReturn(m dsl.Matcher) {
m.Import("github.com/coder/coder/coderd/httpapi")

// Manually enumerate the httpapi function rather then a 'Where' condition
// as this is a bit more efficient.
m.Match(`
if err != nil {
httpapi.Write($*_)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: Does this, or should we, handle the else case as well? How about switch? Is there any way we can express that httapi.Write is inside a block vs function body?

Copy link
Member Author

@Emyrk Emyrk Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that is a good question. This is definitely the common case, but you are right about more possible situations.

I am unsure if I can match if OR else 🤔. This does not work for example

$*_ {
  httpapi.Write($*a)
}

I think I will merge this in as is for now. But expanding it would be nice, I just don't think ruleguard can match different AST nodes very well aside from enumerating all cases. Another idea I had was trying to match the correct case like this:

m.Match(`
httpapi.Write($*a)
$r
`).Where(!m["r"].Text.Matches("return"))

So the next line must be return. This is different than how I've used ruleguard in the past, but might be feasible? I was not able to get it to work with some quick tinkering. It's hard to match for the negative case (missing "return"), and when I've done similar match stuff in the past, the linter starts to become incredibly slow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah interesting, thanks for checking into it. Yup feel free to merge as-is, fixing the other cases is more of a nice-to-have anyway, def. don't want it becoming slow.

`, `
if err != nil {
httpapi.Forbidden($*_)
}
`, `
if err != nil {
httpapi.ResourceNotFound($*_)
}
`).Report("Forgot to return early after the httpapi.Write call")
}

// ProperRBACReturn ensures we always write to the response writer after a
// call to Authorize. If we just do a return, the client will get a status code
// 200, which is incorrect.
Expand Down