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

PatternWildCard hint #1473

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

Conversation

stephenjudkins
Copy link

@stephenjudkins stephenjudkins commented Mar 16, 2023

This scratches an itch for our particular codebase, but I think it's a general rule that might be pretty useful to apply in the right places.

By raising this pull request you confirm you are licensing your contribution under all licenses that apply to this project (see LICENSE) and that you have no patents covering your contribution. ✅

If you care, my PR preferences are at https://github.com/ndmitchell/neil#contributions, but they're all guidelines, and I'm not too fussy - you don't have to read them. ✅

@ndmitchell
Copy link
Owner

Thanks for the patch. What's the harm of using a wildcard in a pattern match? What do you suggest people do instead?

@stephenjudkins
Copy link
Author

Well, in general it's not a problem so I chose to default it to "ignore". But we have a specific case where virutally every instance of discarding part of a data constructor has been a bug. I can imagine there are other contexts where this is the case.

I had planned to write a bespoke script to lint our codebase for just this, but a coworker suggested upstreaming it to hlint instead. If you don't think this is generally applicable enough for that to be appropriate, I apologize for the trouble.

(I can't tell what the CI issue is; the tests appeared to pass locally. Is there anything I can do to address this or is it unrelated to my change?)

@m5
Copy link

m5 commented Apr 4, 2023

Hi Neil! I work with Stephen at Mercury, and wanted to take a moment to talk about the particular use case we have in mind.

There's this pattern in Yesod, where you write an isAuthorized function that takes a route as an argument, and returns whether the current user is allowed to access the route.

So we end up with case statements along the lines of

case route of
  CreatePostR blogId -> blogBelongsToCurrentUser blogId
  EditPostR blogId articleId -> blogBelongsToCurrentUser blogId && articleBelongsToBlog blogId articleId
  ...

These routes end up chock-full of object ids that each really need to be validated.

If we ever see an entry pop up in those case statements with a hole, eg

  DeletePostR blogId _ -> blogBelongsToCurrentUser blogId

we can bet that's going to be a security vulnerability. Somebody forgot to check if the article belongs to the blog, and an attacker is going to be able to delete posts on any blog so long as they know the article id.

So, we've been manually enforcing a policy that we don't allow holes in these case statements. All path parameters should be authorized inside these case statements. If a path parameter really doesn't need to be validated, we ask people to give it a meaningful name, like _status or whatever, and leave a comment.

Stephen's written this PR up with the hope that we can use hlint to help remind people to follow that convention. I wish I had a couple other use cases in mind to help sell you on the feature, but it will at minimum be super valuable for us for this particular use case.

@stephenjudkins stephenjudkins force-pushed the sdj/no-wildcard-patterns branch from b435365 to 57df440 Compare April 12, 2023 06:17
@9999years
Copy link
Contributor

This PR doesn't compile on GHC 9.6 yet. Here's a patch to enable support:

--- a/src/Hint/PatternWildCard.hs
+++ b/src/Hint/PatternWildCard.hs
@@ -1,3 +1,4 @@
+{-# LANGUAGE CPP #-}
 {-
     Warn against wildcards in pattern
 
@@ -22,7 +23,11 @@ patternWildCardHint :: DeclHint
 patternWildCardHint _ _ code = concatMap inspectCode $ childrenBi code
 
 inspectCode :: LHsExpr GhcPs -> [Idea]
+#if __GLASGOW_HASKELL__ >= 906
+inspectCode (L _ ((HsCase _ _ (MG _ (L _ cases))))) = concatMap inspectCase cases
+#else
 inspectCode (L _ ((HsCase _ _ (MG _ (L _ cases) _)))) = concatMap inspectCase cases
+#endif
 inspectCode o = concatMap inspectCode $ children o
 
 inspectCase :: LMatch GhcPs (LHsExpr GhcPs) -> [Idea]

@stephenjudkins
Copy link
Author

Thanks @9999years!

@parsonsmatt
Copy link

Is there anything we can do to help get this merged?

@9999years
Copy link
Contributor

Hey Neil, we're still interested in this. Thanks for your time!

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.

6 participants