From 676792de14fc6cfe21982da982a4f1b35ece59bf Mon Sep 17 00:00:00 2001 From: Mattes Mohr Date: Mon, 4 Sep 2023 21:57:47 +0200 Subject: [PATCH] Harden the HTML output (#141) * Escape characters to prevent xss sinks * Escape the value of html attributes * Differentiate the character escaping of an attribute and an element body * Secure the components as well * Add the option for the autoescaping to the vapor provider With the option, people can decide if they want it or not. --- .../Framework/Rendering/Renderer.swift | 83 +++++++++++++--- .../HTMLKit/Framework/Security/Security.swift | 9 ++ .../Actions/SubmitAction.swift | 4 +- .../Actions/ViewAction.swift | 10 +- .../Extensions/Components+String.swift | 11 +++ .../Extensions/Vapor+HTMLKit.swift | 24 ++++- Sources/HTMLKitVapor/ViewRenderer.swift | 4 +- .../SecurityTests.swift | 77 +++++++++++++++ Tests/HTMLKitTests/PerformanceTests.swift | 26 ++++- Tests/HTMLKitTests/RenderingTests.swift | 16 --- Tests/HTMLKitTests/SecurityTests.swift | 98 +++++++++++++++++++ 11 files changed, 320 insertions(+), 42 deletions(-) create mode 100644 Sources/HTMLKit/Framework/Security/Security.swift create mode 100644 Sources/HTMLKitComponents/Extensions/Components+String.swift create mode 100644 Tests/HTMLKitComponentsTests/SecurityTests.swift create mode 100644 Tests/HTMLKitTests/SecurityTests.swift diff --git a/Sources/HTMLKit/Framework/Rendering/Renderer.swift b/Sources/HTMLKit/Framework/Rendering/Renderer.swift index b38bda06..e9617694 100644 --- a/Sources/HTMLKit/Framework/Rendering/Renderer.swift +++ b/Sources/HTMLKit/Framework/Rendering/Renderer.swift @@ -42,19 +42,31 @@ public class Renderer { private var environment: Environment private var localization: Localization? + + private var security: Security /// Initiates the renderer. public init(localization: Localization? = nil) { + self.localization = localization self.environment = Environment() + self.security = Security() + } + + /// Initiates the renderer. + public init(localization: Localization? = nil, security: Security) { + self.localization = localization + self.environment = Environment() + self.security = security } /// Initiates the renderer. - public init(localization: Localization? = nil, environment: Environment) { + public init(localization: Localization? = nil, environment: Environment, security: Security) { - self.environment = environment self.localization = localization + self.environment = environment + self.security = security } /// Renders a view @@ -112,11 +124,11 @@ public class Renderer { } if let value = content as? EnvironmentValue { - result += try render(value: value) + result += escape(content: try render(value: value)) } if let element = content as? String { - result += element + result += escape(content: (element)) } } @@ -177,11 +189,11 @@ public class Renderer { } if let value = content as? EnvironmentValue { - result += try render(value: value) + result += escape(content: try render(value: value)) } if let element = content as? String { - result += element + result += escape(content: element) } } } @@ -209,12 +221,30 @@ public class Renderer { /// Renders a document element internal func render(element: some DocumentNode) -> String { - return "" + + var result = "" + + result += "" + + return result } /// Renders a comment element internal func render(element: some CommentNode) -> String { - return "" + + var result = "" + + result += "" + + return result } /// Renders a content element @@ -266,8 +296,12 @@ public class Renderer { result += try render(stringkey: stringkey) } + if let value = content as? EnvironmentValue { + result += escape(content: try render(value: value)) + } + if let element = content as? String { - result += element + result += escape(content: element) } } } @@ -348,7 +382,7 @@ public class Renderer { return String(doubleValue) case let stringValue as String: - return String(stringValue) + return stringValue case let dateValue as Date: @@ -374,13 +408,38 @@ public class Renderer { result += " \(attribute.key)=\"" if let value = attribute.value as? EnvironmentValue { - result += "\(try render(value: value))\"" + result += escape(attribute: try render(value: value)) } else { - result += "\(attribute.value)\"" + result += escape(attribute: "\(attribute.value)") } + + result += "\"" } return result } + + /// Converts specific charaters into encoded values. + internal func escape(attribute value: String) -> String { + + if security.autoEscaping { + return value.replacingOccurrences(of: "&", with: "&") + .replacingOccurrences(of: "\"", with: """) + .replacingOccurrences(of: "'", with: "'") + } + + return value + } + + /// Converts specific charaters into encoded values. + internal func escape(content value: String) -> String { + + if security.autoEscaping { + return value.replacingOccurrences(of: "<", with: "<") + .replacingOccurrences(of: ">", with: ">") + } + + return value + } } diff --git a/Sources/HTMLKit/Framework/Security/Security.swift b/Sources/HTMLKit/Framework/Security/Security.swift new file mode 100644 index 00000000..0c4c7bbf --- /dev/null +++ b/Sources/HTMLKit/Framework/Security/Security.swift @@ -0,0 +1,9 @@ +public class Security { + + public var autoEscaping: Bool + + public init() { + + self.autoEscaping = true + } +} diff --git a/Sources/HTMLKitComponents/Actions/SubmitAction.swift b/Sources/HTMLKitComponents/Actions/SubmitAction.swift index 1cf57f6b..ad85448d 100644 --- a/Sources/HTMLKitComponents/Actions/SubmitAction.swift +++ b/Sources/HTMLKitComponents/Actions/SubmitAction.swift @@ -13,10 +13,10 @@ extension SubmitAction { if let data = try? JSONEncoder().encode(validators) { if let result = String(data: data, encoding: .utf8) { - return "$('#\(target)').validate('\(result)');" + return "$('#\(target.escape())').validate('\(result)');" } } - return "$('#\(target)').validate('[]');" + return "$('#\(target.escape())').validate('[]');" } } diff --git a/Sources/HTMLKitComponents/Actions/ViewAction.swift b/Sources/HTMLKitComponents/Actions/ViewAction.swift index 37d3edf7..a7a83ca8 100644 --- a/Sources/HTMLKitComponents/Actions/ViewAction.swift +++ b/Sources/HTMLKitComponents/Actions/ViewAction.swift @@ -21,22 +21,22 @@ public protocol ViewAction { extension ViewAction { public func show(_ target: String) -> String { - return "$('#\(target)').show();" + return "$('#\(target.escape())').show();" } public func hide(_ target: String) -> String { - return "$('#\(target)').hide();" + return "$('#\(target.escape())').hide();" } public func animate(_ target: String) -> String { - return "$('#\(target)').animate();" + return "$('#\(target.escape())').animate();" } public func open(_ target: String) -> String { - return "$('#\(target)').open();" + return "$('#\(target.escape())').open();" } public func close(_ target: String) -> String { - return "$('#\(target)').close();" + return "$('#\(target.escape())').close();" } } diff --git a/Sources/HTMLKitComponents/Extensions/Components+String.swift b/Sources/HTMLKitComponents/Extensions/Components+String.swift new file mode 100644 index 00000000..b131716a --- /dev/null +++ b/Sources/HTMLKitComponents/Extensions/Components+String.swift @@ -0,0 +1,11 @@ +extension String { + + internal func escape() -> String { + + return self.replacingOccurrences(of: "&", with: "&") + .replacingOccurrences(of: "<", with: "<") + .replacingOccurrences(of: ">", with: ">") + .replacingOccurrences(of: "\"", with: """) + .replacingOccurrences(of: "'", with: "'") + } +} diff --git a/Sources/HTMLKitVapor/Extensions/Vapor+HTMLKit.swift b/Sources/HTMLKitVapor/Extensions/Vapor+HTMLKit.swift index 1e406205..fa208d33 100644 --- a/Sources/HTMLKitVapor/Extensions/Vapor+HTMLKit.swift +++ b/Sources/HTMLKitVapor/Extensions/Vapor+HTMLKit.swift @@ -26,6 +26,24 @@ extension Application { public typealias Value = HTMLKit.Environment } + internal struct SecurityStorageKey: StorageKey { + + public typealias Value = HTMLKit.Security + } + + public var security: HTMLKit.Security { + + if let configuration = self.application.storage[SecurityStorageKey.self] { + return configuration + } + + let configuration = Security() + + self.application.storage[SecurityStorageKey.self] = configuration + + return configuration + } + /// The view localization public var localization: HTMLKit.Localization { @@ -59,7 +77,8 @@ extension Application { return .init(eventLoop: application.eventLoopGroup.next(), localization: localization, - environment: environment) + environment: environment, + security: security) } /// The application dependency @@ -94,6 +113,7 @@ extension Request { return .init(eventLoop: self.eventLoop, localization: self.application.htmlkit.localization, - environment: self.application.htmlkit.environment) + environment: self.application.htmlkit.environment, + security: self.application.htmlkit.security) } } diff --git a/Sources/HTMLKitVapor/ViewRenderer.swift b/Sources/HTMLKitVapor/ViewRenderer.swift index d913814b..9c08ae2d 100644 --- a/Sources/HTMLKitVapor/ViewRenderer.swift +++ b/Sources/HTMLKitVapor/ViewRenderer.swift @@ -16,10 +16,10 @@ public class ViewRenderer { internal var renderer: Renderer /// Creates the view renderer - public init(eventLoop: EventLoop, localization: HTMLKit.Localization, environment: HTMLKit.Environment) { + public init(eventLoop: EventLoop, localization: Localization, environment: HTMLKit.Environment, security: Security) { self.eventLoop = eventLoop - self.renderer = Renderer(localization: localization, environment: environment) + self.renderer = Renderer(localization: localization, environment: environment, security: security) } /// Renders a layout and its context diff --git a/Tests/HTMLKitComponentsTests/SecurityTests.swift b/Tests/HTMLKitComponentsTests/SecurityTests.swift new file mode 100644 index 00000000..8a1701fb --- /dev/null +++ b/Tests/HTMLKitComponentsTests/SecurityTests.swift @@ -0,0 +1,77 @@ +import HTMLKit +import HTMLKitComponents +import XCTest + +final class SecurityTests: XCTestCase { + + struct TestView: View { + + @ContentBuilder var body: Content + } + + var renderer = Renderer() + + func testEncodingAttributeContext() throws { + + let attack = "\" onclick=\"alert(1);\"" + + let view = TestView { + Button(role: .button) { + "Show" + } + .tag(attack) + } + + XCTAssertEqual(try renderer.render(view: view), + """ + + """ + ) + } + + func testEncodingActionContext() throws { + + let attack = "test').appendTo(''); $('#test" + + let view = TestView { + Button(role: .button) { + "Show" + } + .tag("sender") + .onClick { button in + button.show(attack) + } + + } + + XCTAssertEqual(try renderer.render(view: view), + """ + \ + + """ + ) + } + + func testEncodingCssContext() throws { + + let attack = "test\" style=\"property: unsafe\"" + + let view = TestView { + Text { + "Text" + } + .backgroundColor(.custom(attack)) + } + + XCTAssertEqual(try renderer.render(view: view), + """ +

Text

+ """ + ) + } +} + diff --git a/Tests/HTMLKitTests/PerformanceTests.swift b/Tests/HTMLKitTests/PerformanceTests.swift index a65366e8..e1a05332 100644 --- a/Tests/HTMLKitTests/PerformanceTests.swift +++ b/Tests/HTMLKitTests/PerformanceTests.swift @@ -7,13 +7,33 @@ import HTMLKit import XCTest final class PerformanceTests: XCTestCase { - - var renderer = Renderer() - func testPerformance() throws { + func testPerformanceWithoutAutoEscaping() throws { + + let context = SampleContext(id: 0, title: "TestPage", excerpt: "Testpage", modified: Date(), posted: Date()) + + let security = Security() + security.autoEscaping = false + + let renderer = Renderer(security: security) + + measure { + + for _ in 0...1000 { + _ = try! renderer.render(view: SampleView(context: context)) + } + } + } + + func testPerformanceWithAutoEscaping() throws { let context = SampleContext(id: 0, title: "TestPage", excerpt: "Testpage", modified: Date(), posted: Date()) + let security = Security() + security.autoEscaping = true + + let renderer = Renderer(security: security) + measure { for _ in 0...1000 { diff --git a/Tests/HTMLKitTests/RenderingTests.swift b/Tests/HTMLKitTests/RenderingTests.swift index d1f35e71..1cc4a5a2 100644 --- a/Tests/HTMLKitTests/RenderingTests.swift +++ b/Tests/HTMLKitTests/RenderingTests.swift @@ -154,22 +154,6 @@ final class RenderingTests: XCTestCase { ) } - func testEscaping() throws { - - let view = TestView { - Paragraph { - "text" - } - .class("cl'ass") - } - - XCTAssertEqual(try renderer.render(view: view), - """ -

text

- """ - ) - } - func testModified() throws { let isModified: Bool = true diff --git a/Tests/HTMLKitTests/SecurityTests.swift b/Tests/HTMLKitTests/SecurityTests.swift new file mode 100644 index 00000000..d985df53 --- /dev/null +++ b/Tests/HTMLKitTests/SecurityTests.swift @@ -0,0 +1,98 @@ +/* + Abstract: + The file tests the rendering of the elements. + */ + +import HTMLKit +import XCTest + +final class SecurityTests: XCTestCase { + + struct TestView: View { + + @ContentBuilder var body: Content + } + + var renderer = Renderer() + + func testEncodingAttributeContext() throws { + + let attack = "\" onclick=\"alert(1);\"" + + let view = TestView { + Paragraph { + } + .class(attack) + } + + XCTAssertEqual(try renderer.render(view: view), + """ +

+ """ + ) + } + + func testEncodingHtmlContext() throws { + + let attack = "" + + let view = TestView { + Paragraph { + attack + } + } + + XCTAssertEqual(try renderer.render(view: view), + """ +

<script></script>

+ """ + ) + } + + func testEncodingCommentContext() throws { + + let attack = "--> + """ + ) + } + + func testEncodingEnvironmentValue() throws { + + struct Attack { + let attribute = "\" onclick=\"alert(1);\"" + let content = "" + } + + @EnvironmentObject(Attack.self) + var attack + + let view = TestView { + Article { + Paragraph { + attack.content + } + Img() + .source(attack.attribute) + } + .environment(object: Attack()) + } + + XCTAssertEqual(try renderer.render(view: view), + """ +
\ +

<script></script>

\ + \ +
+ """ + ) + } +} +