omb-prompt-base: Add "." to git branch whitelist

[hiagofranco]

Add "." to the git branch character whitelist to fix branches like v6.0 or 5.4.2, as examples.

I'm working with a branch that is called "toradex_5.4-2.3.x-imx" from the linux-toradex repo. It was get renamed to "toradex_5-4-2-3-x-imx", which is very annoying. This also passed the https://github.com/njhartwell/pw3nage test.

[akinomyoga]

Thank you for your contribution. It seems to me that we should consider merging this, but I'm not sure why we are now replacing the ref names here in the first place. @hiagofranco @nntoan Do you know why we need to replace the ref names? If there is an actual reason to do so, doesn't the PR change affect it? That is my naive question.

If no one knows the reason, maybe we should stop this entirely. Or we could only replace non-graphic characters and escape sequences.

[hiagofranco]

Hi @akinomyoga, thanks for your message. From my understanding, the refs are being replaced to avoid the shell vulnerability described here https://github.com/njhartwell/pw3nage. However, the "."s can also be excluded from the replacement without breaking the code and exposing this vulnerability. Let me know if I'm wrong.

[hiagofranco]

Thank you for the explanation! That makes sense, and the code provided on the page looks very similar to the very first version of OMB (e65c390b / themes/base.theme.sh:135).

I believe it is enough to escape non-printable characters and $, `. and \. The reason that we replace the non-printable characters is to make it allow breaking the terminal layout (such as CR, ANSI control sequences, escape sequences of ISO-2022 encoding, etc.).

Also, could you add code comments to explain why we are doing this here with the reference you mentioned above?

Thanks for the suggestion, I've added it to the code. Tested here and it works fine. About the comment, there is already a comment on this function:

# This is added to address bash shell interpolation vulnerability described
# here: https://github.com/njhartwell/pw3nage
function git_clean_branch {
  local unsafe_ref=$(command git symbolic-ref -q HEAD 2> /dev/null)
  local stripped_ref=${unsafe_ref##refs/heads/}
  local clean_ref=${stripped_ref//[\$\`\\]/-}
  echo $clean_ref
}

Would you like me to improve this comment?

[akinomyoga]

Thank you for updating!

# This is added to address bash shell interpolation vulnerability described
# here: https://github.com/njhartwell/pw3nage
function git_clean_branch {
  local unsafe_ref=$(command git symbolic-ref -q HEAD 2> /dev/null)
  local stripped_ref=${unsafe_ref##refs/heads/}
  local clean_ref=${stripped_ref//[\$\`\\]/-}
  echo $clean_ref
}

Would you like me to improve this comment?

Ah, OK. I haven't noticed the comment because it was not shown in the diff. Then, the current comment is fine.

Could you also consider adding the escape for non-printable characters as commented in https://github.com/ohmybash/oh-my-bash/pull/420#discussion_r1155222697? Or is there a reason to skip it?

[hiagofranco]

Oh sorry, I forgot to write the updated function in my last answer.

It looks like this now:

function git_clean_branch {
  local unsafe_ref=$(command git symbolic-ref -q HEAD 2> /dev/null)
  local stripped_ref=${unsafe_ref##refs/heads/}
  local clean_ref=${stripped_ref//[\$\`\\]/-}
  clean_ref=${clean_ref//[^[:print:]]/-} # strip escape sequences, etc.
  echo $clean_ref
}

Is it ok? Or do you mean to add something else?

[hiagofranco]

Thank you for the suggestions and help!

[akinomyoga]

I've merged. Thank you!