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
proposal: encoding/csv: CSV Injection #58849
Comments
What specific changes are you proposing to encoding/csv? |
A cell should always be escaped when the following case occurs.
Alternatively, one could provide an option to escape all cells.
Or both. |
Option 1 does not look like a reasonable thing to implement for a generic package like encoding/csv. the package does wrap/escape quotes and separators, it would be sufficient for you to church for leading characters yourself. Option 2 changes the underlying data to always begin with |
Cells containing quotes are escaped again.
Therefore, I cannot use this package as it is. |
I think this way I can still use the package. Thanks for the effort! import (
"bufio"
"encoding/csv"
"fmt"
)
func NewCSVWriter(b *bufio.Writer, comma rune) CSVWriter {
w := csv.NewWriter(b)
w.Comma = comma
return CSVWriter{
Writer: w,
}
}
type CSVWriter struct {
Writer *csv.Writer
}
func (w CSVWriter) Error() error {
return w.Writer.Error()
}
func (w CSVWriter) WriteAll(records [][]string) error {
for _, record := range records {
err := w.Write(record)
if err != nil {
return err
}
}
w.Writer.Flush()
return w.Writer.Error()
}
func (w CSVWriter) Flush() {
w.Writer.Flush()
}
func (w CSVWriter) Write(record []string) error {
for i, each := range record {
if len(each) == 0 {
continue
}
switch each[0] {
case '=', '+', '-', '@', '\t', '\r':
record[i] = fmt.Sprintf(`'%s`, each)
}
}
return w.Writer.Write(record)
} |
Looking at what you've implemented this looks out of scope for encoding/csv, which implements RFC 4180. |
RFC 4180 does state (emphasis added):
It would be possible to implement OWASPs guidance if we added a new option to the CSV writer to quote all fields (overriding the behaviour of Could we add a new option to the writer type Writer struct {
Comma rune // Field delimiter (set to ',' by NewWriter)
UseCRLF bool // True to use \r\n as the line terminator
QuoteAllFields bool // True to quote all fields, even if not required by RFC 4180
w *bufio.Writer
} Then // If we don't have to have a quoted field then just
// write out the field and continue to the next field.
- if !w.fieldNeedsQuotes(field) {
+ if !w.QuoteAllFields && !w.fieldNeedsQuotes(field) { Either way this is RFC 4180 compliant, but it gives users the option to opt-in to quoting all fields to better encapsulate alternative delimiters that might be interpreted by certain spreadsheet programs. |
we declined that in #12755 |
The reason cited is complexity, but it would be a one line change unless I'm missing something? |
The alternative for folks that want to output slightly safer CSVs is to roll a custom CSV writer, which I guess is possible but not something I'm super enthusiastic about deviating from the standard library on 😅 |
There are so many variants of CSV that we decided a long time ago to keep the standard library package only focused on RFC 4180, as the package documentation states. We kept the options that had already been added, but we aren't going to add any more. The package is short and easy to copy for people who need different formats. |
The CSV Writer does not provide protection against CSV injection. I think this should be implemented in the standard library so that you don't have to resort to external packages.
https://owasp.org/www-community/attacks/CSV_Injection
The text was updated successfully, but these errors were encountered: