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

No warnings for invalid syntax submissions #20

Open
devidw opened this issue Sep 10, 2022 · 5 comments
Open

No warnings for invalid syntax submissions #20

devidw opened this issue Sep 10, 2022 · 5 comments

Comments

@devidw
Copy link

devidw commented Sep 10, 2022

I noticed that invalid syntax gets silently submitted without any warnings.

Would be great to get notified about these syntax issues in submitted goggles.

This way, authors could fix them in their source files.

So, the goggle source code displayed on the brave search about page would equal the hosted code.

Here are some of the issues which don't result in warnings:

Invalid Value

Options

$discard=4,site=msn.com

https://github.com/Killthenews/brave/blob/de522bcc456646547f815a3de9ad4fcfa3494f35/news#L11-L18

Metadata

! public: foo
  • Expected behavior: Failing submission
  • Actual behavior: Gets submitted as private goggle

Illegal Range Value

Same for out of scope values for the action options:

$boost=30,site=github.com

https://github.com/9ktz/googles-technical/blob/cb75f9a9e4a9ce2f1d0a3304dbfddc57674fb96d/github.google#L8-L10

Multiple $'s

Same for having multiple $ inside the options part of the instructions.

bill_id=201720180SB1121$inurl,$boost=8

https://github.com/clening/DataProtectionGoggle/blob/19a3f9fd4bc9543c8f45ebfe4d0780826dc0ac18/dataprotection.goggle#L85

@devidw devidw changed the title No warning on wrong discard= syntax No warnings for invalid syntax submissions Sep 10, 2022
@remusao
Copy link
Collaborator

remusao commented Sep 15, 2022

Hi @devidw,

Thanks for opening this issue, that's great feedback! We'll look into it. Note, though that forbidding bill_id=201720180SB1121$inurl,$boost=8 might be problematic if you ever want to match a $ character in a URL (although I guess that might not be very common). Maybe we could show some lint messages in such cases instead of hard errors. Wdyt?

@devidw
Copy link
Author

devidw commented Sep 15, 2022

Hey @remusao

Happy to hear that 🙂

When talking about these cases with multiple $’s inside the instruction lines, I guess this fully depends on how they are interpreted by Brave.

For example, taking this example:

a$inurl,$boost

Would this be interpreted as:

  • No (unescaped?) $ allowed in pattern:
    • Pattern: a
    • Options: inurl, boost

Or:

  • Valid options string after last $:
    • Pattern: a$inurl,
    • Options: boost

Or even differently?

Perhaps escape characters for goggle syntax critical tokens could help here?

@remusao
Copy link
Collaborator

remusao commented Oct 23, 2022

Hi @devidw,

a$inurl,$boost

Would be interpreted as:

  • Pattern: a$inurl,
  • Options: boost

Not sure if escaping would help since it's not really expected for a $ to be part of the options (it's not valid as part of a domain name) so it would not really make sense to have $ in options I think.

Wdyt?

@devidw
Copy link
Author

devidw commented Oct 24, 2022

@remusao alright, I see.

In this case what I pointed out as error "Multiple $'s" would be indeed not an error at all but valid syntax.

The only problematic situation I could image would be someone writing an instruction with multiple dollar signs and don't want to add options to it. If the last part of the pattern matches one option, it would not be interpreted as pattern, but rather as instruction.

For example, if we want to write a pattern like:

a$boost

And actually want to have it interpreted as a$boost and not as only a.

The workaround would be to $boost to the end.

a$boost$boost

This would not change the results since boost is the default behavior.

And the final pattern would be the expected a$boost.

Perhaps it's even possible to add a single $ to the end and since it's the last one, it would be interpreted as separator for the options but wouldn't do anything. The only role would be to prevent the dollar sign before that one to be interpreted as option.

So something like a little hacky:

a$boost$

In case this works as described?

Alternatively, escaping in the pattern could help, so there would be no need to add a tailing dollar sign.

a\$boost

But I don't know if it makes sense to implement escaping because of the possibly unnecessary complexity overload.

@remusao
Copy link
Collaborator

remusao commented Oct 24, 2022

Adding a $ at the end might already work, actually. I did not check but I expect the rule should be parsed correctly in the way you described in your example.

@ghost ghost mentioned this issue Dec 14, 2022
Closed
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

No branches or pull requests

2 participants