Skip to content

Use shlex for parsing policy::Table#157

Closed
markafarrell wants to merge 1 commit into
openpubkey:mainfrom
markafarrell:feature/table-support-quoting-shlex
Closed

Use shlex for parsing policy::Table#157
markafarrell wants to merge 1 commit into
openpubkey:mainfrom
markafarrell:feature/table-support-quoting-shlex

Conversation

@markafarrell
Copy link
Copy Markdown
Contributor

This replaces the current approach for parsing policy (space delimited) with shlex. This allows us to include spaces in policy fields.

This is important for:

  • policy matching for groups that include spaces
  • the future use of CEL

shlex does NOT support quoting fields that include spaces and as a result doing something like:

e.g.

t := NewTable("1 \"2 3\" 4") // [ [ "1", "2 3", "4" ] ]
s := t.ToString() // "1 2 3 4"
t1 := NewTable(s) // [ [ "1", "2", "3", "4"] ]

It this is important we can use shellquote

@markafarrell
Copy link
Copy Markdown
Contributor Author

It looks like it is critical that we preserve the quoting as it is used to write the policy file so I think we are better off using shellquote #158 instead

@yonatan-sevenai
Copy link
Copy Markdown

yonatan-sevenai commented Apr 22, 2025

This implementation is broken since it uses shlex to read tables but join(row, " ") to write.
This means that writing oidc:groups:some group will read oidc:group:some as the write function won't add the proper escape functions. The shellquote approach #158 looks more complete.

See my comment here

@EthanHeilman
Copy link
Copy Markdown
Member

Do we want to close this? It sounds like everyone wants shellquote instead?

@yonatan-sevenai
Copy link
Copy Markdown

As long as we're comfortable with the background compatibility, yes - shellquote would be superior.

@markafarrell
Copy link
Copy Markdown
Contributor Author

Close as shellquote is better

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.

3 participants