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.

Sign up for GitHub

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

Refactor span formatter function to use a html template to generate autoescaped HTML #5280

Closed
wants to merge 4 commits into from
Closed

Refactor span formatter function to use a html template to generate autoescaped HTML #5280

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a80e3cf
modified FN in templates.go to use auto-escaping HTML
AkhigbeEromo Mar 15, 2024
1febcd6
removed comment
AkhigbeEromo Mar 15, 2024
51899df
updated template.go
AkhigbeEromo Mar 15, 2024
2f960f3
updated changelog
AkhigbeEromo Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Added

- Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108)
- Add a new package `go.opentelemetry.io/contrib/zpages/internal` to enable the use of internal.Templates within the parseTemplate function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be removed. It is not included in these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted

- Refactored the spanRowFormatter function to utilize HTML templates for auto-escaping HTML, ensuring protection against Cross-site Scripting (XSS) vulnerabilities. (#4451)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rewrite this in terms that an end-user will understand and move to the "### Fixed" section.

Reference: https://keepachangelog.com/en/1.1.0/


### Removed

Expand Down
17 changes: 14 additions & 3 deletions zpages/templates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package zpages // import "go.opentelemetry.io/contrib/zpages"

import (
"bytes"
"fmt"
"html/template"
"io"
Expand Down Expand Up @@ -60,7 +61,6 @@
return template.Must(template.New(name).Funcs(templateFunctions).Parse(string(text)))
}

//nolint:gosec // G203: The used method does not auto-escape HTML. Tracked under https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4451.
func spanRowFormatter(r spanRow) template.HTML {
if !r.SpanContext.IsValid() {
return ""
Expand All @@ -69,10 +69,21 @@
if r.SpanContext.IsSampled() {
col = "blue"
}

var tpl string

Check warning on line 73 in zpages/templates.go

View check run for this annotation

Codecov / codecov/patch

zpages/templates.go#L73 

Added line #L73 was not covered by tests

if r.ParentSpanContext.IsValid() {
return template.HTML(fmt.Sprintf(`trace_id: <b style="color:%s">%s</b> span_id: %s parent_span_id: %s`, col, r.SpanContext.TraceID(), r.SpanContext.SpanID(), r.ParentSpanContext.SpanID()))
tpl = fmt.Sprintf(`trace_id: <b style="color:%s">%s</b> span_id: %s parent_span_id: %s`, col, template.HTMLEscapeString(r.SpanContext.TraceID().String()), template.HTMLEscapeString(r.SpanContext.SpanID().String()), template.HTMLEscapeString(r.ParentSpanContext.SpanID().String()))
} else {
tpl = fmt.Sprintf(`trace_id: <b style="color:%s">%s</b> span_id: %s `, col, template.HTMLEscapeString(r.SpanContext.TraceID().String()), template.HTMLEscapeString(r.SpanContext.SpanID().String()))

Check warning on line 78 in zpages/templates.go

View check run for this annotation

Codecov / codecov/patch

zpages/templates.go#L76-L78 

Added lines #L76 - L78 were not covered by tests
}

t := template.Must(template.New("spanRow").Parse(tpl))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would you advise i do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really grateful for your corrections @MrAlias

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be better if i just
return template.HTML(fmt.Sprintf(tpl))

var buf bytes.Buffer
if err := t.Execute(&buf, nil); err != nil {
return template.HTML(template.HTMLEscapeString(fmt.Sprintf("Error executing template: %s", err.Error())))

Check warning on line 84 in zpages/templates.go

View check run for this annotation

Codecov / codecov/patch

zpages/templates.go#L81-L84 

Added lines #L81 - L84 were not covered by tests
}
return template.HTML(fmt.Sprintf(`trace_id: <b style="color:%s">%s</b> span_id: %s`, col, r.SpanContext.TraceID(), r.SpanContext.SpanID()))
return template.HTML(template.HTMLEscapeString(buf.String()))

Check warning on line 86 in zpages/templates.go

View check run for this annotation

Codecov / codecov/patch

zpages/templates.go#L86 

Added line #L86 was not covered by tests
}

func even(x int) bool {
Expand Down
Loading