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

Spelling #3083

Closed
wants to merge 50 commits into from
Closed

Spelling #3083

wants to merge 50 commits into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Aug 18, 2021

This PR corrects misspellings identified by the check-spelling action.

The misspellings have been reported at jsoref@c6de09c#commitcomment-55052899

The action reports that the changes in this PR would make it happy: jsoref@2a5bd29

Note: this PR does not include the action. If you're interested in running a spell check on every PR and push, that can be offered separately.

@kingthorin
Copy link
Member

I’ll dig into this further and make a checklist (as we did with the previous round) tomorrow.

@kingthorin
Copy link
Member

There are definitely some paths that are third party and should be excluded, what’s the best way to deal with that?

@jsoref
Copy link
Contributor Author

jsoref commented Aug 18, 2021

Give me a list, I'll add them to excludes and I'll rebase to drop them.

Also, if you can identify the source, I can queue fixing them too.

@thc202
Copy link
Member

thc202 commented Aug 18, 2021

Better split the commits per add-on rather than word, the changelogs should be updated.

@kingthorin
Copy link
Member

kingthorin commented Aug 18, 2021

Exclusions:

  • /imagelocationscanner/src/main/java/com/veggiespam/imagelocationscanner/
  • /tokengen/src/main/java/com/fasteasytrade/JRandTest/

For the third I don't think there's anywhere else to submit the fixes, for the second you could submit them (if any) here: https://github.com/veggiespam/ImageLocationScanner

@jsoref
Copy link
Contributor Author

jsoref commented Aug 19, 2021

@kingthorin I've dropped both things.


I've forked the imagelocationscanner, and I don't think that this code:


is present there.


Afaict, src.com.fasteasytrade.jrandtest was http://jrandtest.sourceforge.net/ in 2005.

I can't find Zur Aougav on the internet after 2005...

There was a fork in 2012 for a few dozen cleanup commits by @cryptopony / @joe-invincible: https://github.com/cryptopony/jrandtest

I can't find any activity for this entity post 2012.

@kingthorin
Copy link
Member

For ILS my main concern was that https://github.com/zaproxy/zap-extensions/blob/main/addOns/imagelocationscanner/src/main/java/com/veggiespam/imagelocationscanner/ILS.java is sourced from https://github.com/veggiespam/ImageLocationScanner/blob/master/src/com/veggiespam/imagelocationscanner/ILS.java.
So just that '"package" (src/main/java/com/veggiespam/imagelocationscanner/) with its one source file should be excluded.

OH I think I buggered up the path earlier, it seems to have an extra leading "imagelocationscanner".....sorry!

@kingthorin
Copy link
Member

@jsoref is it possible to re-package this per add-on (project) as @thc202 mentioned?

jsoref added 3 commits August 24, 2021 17:40
* currency
* inferring
* successfully

Signed-off-by: Josh Soref <[email protected]>
* not tested
* occurred

Signed-off-by: Josh Soref <[email protected]>
* exception

Signed-off-by: Josh Soref <[email protected]>
@jsoref
Copy link
Contributor Author

jsoref commented Aug 24, 2021

For my reference, I used something approximating this to split this PR:

a=$(git status|grep modified:|head -1|perl -pne 's{^\s*modified:\s*([^/]+/[^/]+).*}{$1}');
git add $a;
git commit -m 'spelling: '$a'

'"$(git log --oneline --graph main..jsoref/spelling -- $a|sort -k3|perl -pne 's/.*:/*/')"'

Signed-off-by: Josh Soref <[email protected]>'; 

@kingthorin
Copy link
Member

Thanks @jsoref!

@kingthorin
Copy link
Member

kingthorin commented Sep 2, 2021

$ for ADDON in `git log -51 --pretty=oneline|cut -d' ' -f3|tac`; do echo "- [ ] $ADDON";done
  • addOns/accessControl
  • addOns/alertFilters - I'm unsure about the Not Tested one, as it is nothing will break, however the name of the resource message no longer aligns with the content....
  • addOns/amf
  • addOns/ascanrules - thishouldnonexistentandhopefullyitwillnot doesn't make sense (everything else seems fine to me).
  • addOns/ascanrulesAlpha - ascanalpha.ldapinjection.apachedirectoryservices.errormessages should not be updated, these strings were extracted from source code at some point (everything else seems fine to me).
  • addOns/ascanrulesBeta
  • addOns/authstats - The notsuccess change might be problematic. (Good catch on the icon.)
  • addOns/automation - The notstarted change might be problematic (or need to be extended further, ex: into the switch/case, etc. Everything else seems fine to me).
  • addOns/bruteforce
  • addOns/bugtracker
  • addOns/callgraph
  • addOns/commonlib
  • addOns/diff
  • addOns/domxss
  • addOns/encoder
  • addOns/formhandler - Is it just me or does "inputted" seem awkward? The field name inputted is not case sensitive shall we just use "input" or even drop the word altogether?
  • addOns/frontendscanner
  • addOns/fuzz - Seems fine, should *.jbrf be updated elsewhere? (3rd Party?)
  • addOns/fuzzdb - Maybe these should be done against https://github.com/fuzzdb-project/fuzzdb and picked up the next time we update?
  • addOns/graaljs
  • addOns/graphql
  • addOns/highlighter
  • addOns/importLogFiles
  • addOns/invoke
  • addOns/jython
  • addOns/openapi
  • addOns/plugnhack - PlugnHack is correct, it's like plug-and-hack/plug-n-hack.
  • addOns/portscan - I'm not sure about the changes in Messages.properties, ex: the 777 change seems to be used multiple places (ex: https://www.google.com/search?client=firefox-b-d&q=777+Multiling+HTTP). I'm not sure as to the original source of the port details, but either some of these typos are all over the 'net or they're names and purposefully misspelled?
  • addOns/pscanrules
  • addOns/pscanrulesAlpha
  • addOns/pscanrulesBeta
  • addOns/quickstart - Seems okay, I'm just wondering if the enum changes will be problematic?
  • addOns/replacer
  • addOns/retire - This is a name (https://github.com/nikmmy), it should be left as-is as far as I can tell.
  • addOns/reveal
  • addOns/saml
  • addOns/scripts
  • addOns/selenium
  • addOns/spiderAjax
  • addOns/sqliplugin
  • addOns/sse
  • addOns/tips
  • addOns/todo
  • addOns/tokengen
  • addOns/treetools
  • addOns/viewstate
  • addOns/wappalyzer - I've also submitted this fix upstream. https://github.com/AliasIO/wappalyzer/pull/4555#event-5251067926
  • addOns/websocket
  • addOns/zest - Looks fine, just not sure about the *.jbrf changes.
  • testutils

@kingthorin
Copy link
Member

I got through the A section, will try to do more tomorrow.

@kingthorin
Copy link
Member

I've made it through the Rs. Hopefully more later this evening, or tomorrow.

@jsoref
Copy link
Contributor Author

jsoref commented Sep 2, 2021

For reference, the name thing was me picking poorly to match the more common instance (common because each translation had the spelling): https://github.com/zaproxy/zap-extensions/compare/e714b2de23dffdd2ab924ba35a3969dbe620a0d2..c32b107189f75708155ac4a977e7fa2fc49fe74e#diff-ed10369a90a008893244ea3c7bbe8839bccee40d462eb2ca2a9f0319f901f024L17

@kingthorin
Copy link
Member

The build/test failure can be fixed via:

diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java
index 1db9fd37b..afae3253a 100644
--- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java
+++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java
@@ -374,7 +374,7 @@ class PathTraversalScanRuleUnitTest extends ActiveScannerTest<PathTraversalScanR
         @Override
         protected Response serve(IHTTPSession session) {
             String value = getFirstParamValue(session, param);
-            if (value.equals("thishouldnonexistentandhopefullyitwillnot") && passInitialCheck) {
+            if (value.equals("ThisShouldNotExistAndHopefullyItWillNot") && passInitialCheck) {
                 return newFixedLengthResponse("Error");
             }
             return newFixedLengthResponse(content);

@kingthorin
Copy link
Member

I've made it through everything and updated earlier checklist comments/entries.

@jsoref
Copy link
Contributor Author

jsoref commented Sep 3, 2021

  • addOns/formhandler - Is it just me or does "inputted" seem awkward? The field name inputted is not case sensitive shall we just use "input" or even drop the word altogether?

I've dropped inputted entirely, yes that awkward word is a good hint at not wanting to be used at all.

* access
* criteria
* datadoesnotexist
* earlier
* expression
* for our
* initial
* launch
* reflected
* response
* should
* stripped
* targeted
* vulnerabilities

Signed-off-by: Josh Soref <[email protected]>
* abcdefghijklmnopqrstuvwxyz
* boolean
* empty
* forwarded
* implementation
* internationalization
* logs
* parentheses
* penetration
* threshold

Signed-off-by: Josh Soref <[email protected]>
* 0123456789
* abcdefghijklmnopqrstuvwxyz
* analyzed
* attack
* attribute
* comparison
* concurrency
* consecutive
* constructor
* entry
* heartbeat
* hirshberg
* identifiable
* language
* length
* management
* mysql
* naive
* nonexistentfilemsg
* nonexistentparentmsg
* padding
* param
* query
* read
* remote
* repositories
* response
* specific
* successfully
* vulnerabilities

Signed-off-by: Josh Soref <[email protected]>
* lock
* unsuccessful

Signed-off-by: Josh Soref <[email protected]>
* additional
* contexts
* extension
* include
* pending
* pluggable
* progress
* threshold
* unknown

Signed-off-by: Josh Soref <[email protected]>
@kingthorin
Copy link
Member

kingthorin commented Sep 3, 2021

When I said ‘drop the word altogether’ I meant the word in that sentence (it didn’t need input, inputted, etc at all it made sense without it).

However if you think dropping it totally is a good move too then I’m fine with that.

@jsoref
Copy link
Contributor Author

jsoref commented Sep 6, 2021

fuzzdb-project/fuzzdb#201 (I didn't cross check to confirm that everything from this PR is in there, I probably should, but ...)

@kingthorin
Copy link
Member

Hi @jsoref, just so you know I'm going to work on breaking this up/cherry picking parts and getting them merged.

As you can see here there's quite a few conflicts from the code moving along, which is fine but I'd like to get a bunch of this done where it isn't controversial. As I work through things I'll try to maintain your authorship in the git info and create the PRs myself (so don't be surprised if you get notifications that seem slightly confusing 😉 )

@kingthorin kingthorin self-assigned this Dec 13, 2022
@jsoref
Copy link
Contributor Author

jsoref commented Dec 13, 2022

I'm absolutely fine w/ that. And I don't particularly care about losing attribution. My general goal is to not run into spelling errors when I'm using programs or their documentation or their samples or their derivatives or suffer from their consequences when trying to debug problems stemming from them.

Thanks for taking this on.

@thc202
Copy link
Member

thc202 commented Dec 22, 2022

How many more batches are pending?

@kingthorin
Copy link
Member

Umm probably two. I haven’t looked deeply yet. But still need to tackle the 6 rule add-ons.

This was referenced Dec 22, 2022
@kingthorin
Copy link
Member

I believe this can be closed now.

  • fuzzdb will be dealt with after the upstream issue is sorted.
  • the jbrf changes in fuzz and zest will be dealt with later when we decide how we want to handle that without duplication.

@thc202
Copy link
Member

thc202 commented Dec 30, 2022

Thank you both again!

@thc202 thc202 closed this Dec 30, 2022
@jsoref
Copy link
Contributor Author

jsoref commented Dec 30, 2022

I'm going to run a new pass to see how things went, there are a couple of places where it looks like whitespace wrapping appears to be harsher than it should have been.

@jsoref jsoref deleted the spelling branch December 30, 2022 14:48
@thc202
Copy link
Member

thc202 commented Dec 30, 2022

there are a couple of places where it looks like whitespace wrapping appears to be harsher than it should have been.

Could you provide an example?

@kingthorin
Copy link
Member

That might have been me manually wrapping things, if you're referring to HTML.

@jsoref
Copy link
Contributor Author

jsoref commented Dec 30, 2022

@kingthorin
Copy link
Member

Oh I suspect that's just spotlessApply wrapping to length but not being able to consider context. (Ex: Pulling up/back the following line)

@jsoref
Copy link
Contributor Author

jsoref commented Dec 30, 2022

Yeah, it felt like a robot.

@jsoref
Copy link
Contributor Author

jsoref commented Dec 30, 2022

So, check-spelling is tunable, but w/ some default settings, here are the special things it doesn't like:
https://github.com/check-spelling/zap-extensions/actions/runs/3808821577#summary-10368675301
And, more, or, less, here are the words that it found that it doesn't recognize on its own:
https://github.com/check-spelling/zap-extensions/blob/4e6e4864ae1bef61e6edb14bb923198858f5fc91/.github/actions/spelling/expect.txt
I haven't scanned the list, but there's at least one definite typo in the first 20 items on the list and at least a second in the next 20 items.

The list comes from files that aren't excluded and text in lines that isn't matched by a pattern.

I don't have any time left for the day. I suspect that means I won't look further until spring...

beyond that, here are some leftovers from this PR that seem like they might want to be consumed:
check-spelling-sandbox@d208a8b
check-spelling-sandbox@b211495
check-spelling-sandbox@7e42d2b
check-spelling-sandbox@266879f
check-spelling-sandbox@e7f5ff4
check-spelling-sandbox@dc9b78b -- the main thing here is that thishould is one s short -- it should be this+should.

@thc202
Copy link
Member

thc202 commented Dec 30, 2022

it should be this+should.

Why? That doesn't have to be spelt correctly (and I'd argue being as is is less likely to exist than if it was correctly spelt).

@kingthorin
Copy link
Member

@jsoref thanks for running things again. I'll fire in another PR with a few tweaks as I go through things.

@kingthorin kingthorin mentioned this pull request Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants