Add HTTP URLs verification by default #14

JoaoOliveiraRocha · 2021-04-28T22:01:10Z

Attempt to cover #11. Possible breaking changes.

It now verifies if it's an HTTP URL using a RegEx based on this

You can now use isAbsolutePath('http://sindresorhus.com'). It will behave as isAbsolutePath('http://sindresorhus.com', true).

There is possible to isAbsolutePath('http://sindresorhus.com', false) and get the same results as before this PR (it even uses the original RegEx to evaluate the string).

I've decided to K.I.S.S. and keep the PR as you mentioned here, in #11 and #12. Since there are no requests for extra features there's no need to "prepare" for expansion right now.

Note: this package returns true on situations like http://sindr esorhus.com (white-space in between). I kept its original behavior. If you think it would be helpful for this package to check for whitespaces (and warn/return false) please inform me!

~~This last object, called Options, can have some parameters (so it's easier to add extra features later). Right now I've only done 'httpOnly'.~~

It has 3 levels of 'strictness': 'strictest', 'strict' and 'loose' / 'true. Basically strictest is the rigid http://www.lorem.ipsum.com/path, the strict is something your browser could understand, and loose only checks for the HTTP protocol header.

~~I have wrote some examples on the /readme.md file.~~

I've also copied the current array of tests to test against the 'levels of strictness'. If you have any doubt about each link I suggest open the test files and see what is the result of each "URL" against each parameter.

Fixes #11

sindresorhus · 2021-04-29T09:13:02Z

readme.md

 ```

+
+


Don't do unrelated changes. Applies everywhere.

sindresorhus · 2021-04-29T09:14:32Z

index.d.ts

@@ -17,6 +17,6 @@ isAbsoluteUrl('foo/bar');
 //=> false
 ```
 */
-declare function isAbsoluteUrl(url: string): boolean;
+declare function isAbsoluteUrl(url: string, options: object): boolean;


This should be strongly-typed. See: https://github.com/sindresorhus/typescript-definition-style-guide/tree/57a925bb5b3eb46a4666eab31e1c883a26b7bfb5#documentation Should also have docs.

sindresorhus · 2021-04-29T09:15:23Z

There are a lot of typos and formatting problems. Try to look over the diff and improve it: https://github.com/sindresorhus/is-absolute-url/pull/14/files

sindresorhus · 2021-04-29T09:19:44Z

Why 3 states instead of just a httpOnly option as mentioned in #11?

JoaoOliveiraRocha · 2021-04-29T11:48:49Z

Why 3 states instead of just a httpOnly option as mentioned in #11?

It's possible to simply set httpOnly as true and it will behave the way #11 intended.

I've added the 3 states because 1) according to the RFC: URLs should be case-sensitive. And 2) it could be expanded in the future to check other conditions p.e. if is an absolute ftp address, contains the port or to check for secure HTTPS only ( p.e. pass { httpOnly: true, portOnly: true } for validating a web link with a defined port ).

Or do you think it's better to keep it simple and go in a way #12 tried to archive?

satazor

There are a lot of unrelated changes in the docs.

satazor · 2021-04-29T12:10:32Z

index.js

@@ -1,16 +1,35 @@
 'use strict';

-module.exports = url => {
+// Options - httpOnly


Suggested change

// Options - httpOnly

// Options

// - httpOnly

satazor · 2021-04-29T12:11:12Z

readme.md

@@ -26,19 +38,90 @@ isAbsoluteUrl('foo/bar');
 ```


-## Related
+## Options Parameters  


Suggested change

## Options Parameters

## Options

readme.md

satazor · 2021-04-29T12:14:50Z

readme.md


-See [is-relative-url](https://github.com/sindresorhus/is-relative-url) for the inverse.
+### httpOnly  
+


I would keep the docs simple like how it's done in the Usage section. A simple js block with lines and their result as comments would be better for readability.

Understood. Can you provide feedback now please?

sindresorhus · 2021-05-02T13:40:49Z

I think it should be a single httpOnly boolean option.

sindresorhus · 2021-05-02T13:43:39Z

strictest > Everything accordingly to RFC 1738 - Case sensititive

This is incorrect. The URL scheme is case-insensitive.

sindresorhus · 2021-05-02T13:45:25Z

You missed this from the original issue:

How about an option called httpOnly which defaults to true

sindresorhus · 2021-05-02T13:36:26Z

index.d.ts

@@ -17,6 +17,10 @@ isAbsoluteUrl('foo/bar');
 //=> false
 ```
 */
-declare function isAbsoluteUrl(url: string): boolean;
+interface Options {


This needs to be inside a namespace: https://github.com/sindresorhus/typescript-definition-style-guide

sindresorhus · 2021-05-02T13:36:34Z

index.d.ts

@@ -17,6 +17,10 @@ isAbsoluteUrl('foo/bar');
 //=> false
 ```
 */
-declare function isAbsoluteUrl(url: string): boolean;
+interface Options {
+    httpOnly?: boolean | string;


Tab-indentation

sindresorhus · 2021-05-02T13:36:51Z

index.d.ts

 @param url - The URL to check.
+@param options - Object with different parameters to check.


This doesn't add much. I would remove it. People know what "options" means.

sindresorhus · 2021-05-02T13:36:56Z

index.d.ts

@@ -1,7 +1,7 @@
 /**
 Check if a URL is absolute.
-


Don't do unrelated changes.

sindresorhus · 2021-05-02T13:37:56Z

readme.md

@@ -41,4 +56,4 @@ See [is-relative-url](https://github.com/sindresorhus/is-relative-url) for the i
 	<sub>
 		Tidelift helps make open source sustainable for maintainers while giving companies<br>assurances about security, maintenance, and licensing for their dependencies.
 	</sub>
-</div>
+</div>


Don't do unrelated changes.

sindresorhus · 2021-05-02T13:43:58Z

index.test-d.ts

@@ -1,4 +1,4 @@
 import {expectType} from 'tsd';
 import isAbsoluteUrl = require('.');

-expectType<boolean>(isAbsoluteUrl('https://sindresorhus.com/foo/bar'));
+expectType<boolean>(isAbsoluteUrl('https://sindresorhus.com/foo/bar', { httpOnly: true }));


Suggested change

expectType<boolean>(isAbsoluteUrl('https://sindresorhus.com/foo/bar', { httpOnly: true }));

expectType<boolean>(isAbsoluteUrl('https://sindresorhus.com/foo/bar', {httpOnly: true}));

sindresorhus · 2021-05-02T13:45:58Z

readme.md

+isAbsoluteUrl('https://si', { httpOnly: true });
+//=> true
+
+is absoluteUrl('mailto:[email protected]', { httpOnly: true });


sindresorhus · 2021-05-02T13:46:32Z

readme.md

@@ -25,6 +25,21 @@ isAbsoluteUrl('foo/bar');
 //=> false
 ```

+## Options


Use the same format for readme docs as used here: https://github.com/sindresorhus/boxen#api

sindresorhus · 2021-05-02T13:47:11Z

test.js

+	t.false(isAbsoluteUrl('C:\\Dev\\test-broken', {}));
+	t.false(isAbsoluteUrl('ht,tp://sindresorhus.com', {}));
+});
+


Suggested change

sindresorhus · 2021-05-02T13:47:41Z

The pull request needs a proper descriptive title.

JoaoOliveiraRocha · 2021-05-02T16:53:07Z

You missed this from the original issue:

How about an option called httpOnly which defaults to true

Done. I've also changed the title and description of this PR.

sindresorhus · 2021-05-04T12:56:29Z

index.js

+// - true : checks http(s) header and correct formatting or URL.
+// - false: checks only accordance with rfc3986
+
+module.exports = (url, httpOnly = true) => {


httpOnly should be an options-object, not a parameter.

https://ariya.io/2011/08/hall-of-api-shame-boolean-trap

index.d.ts

sindresorhus · 2021-05-04T12:57:51Z

index.js

 	if (typeof url !== 'string') {
 		throw new TypeError(`Expected a \`string\`, got \`${typeof url}\``);
 	}

+	// Optional Chaining not supported by XO? please confirm


Code comments is not the place to discuss PR stuff. You can just comment on your own PR if you have questions.

sindresorhus · 2021-05-04T12:57:59Z

index.js

 	if (typeof url !== 'string') {
 		throw new TypeError(`Expected a \`string\`, got \`${typeof url}\``);
 	}

+	// Optional Chaining not supported by XO? please confirm
+	if (httpOnly) {
+		return /^(((([hH][tT]{2}[pP]([sS])?)?:\/\/)(www\.)?)([-a-zA-Z0-9()@:%_.~#?&//=]*))/g.test(url);


The regex can be simplified.

sindresorhus · 2021-05-04T12:58:19Z

index.js

-module.exports = url => {
+// Param: httpOnly
+// - true : checks http(s) header and correct formatting or URL.
+// - false: checks only accordance with rfc3986


Remove these comments. They don't add anything.

sindresorhus · 2021-05-04T12:58:54Z

readme.md

+Type: `boolean`
+Default: `true`
+
+Check if a URL is a http URL. Defaults to `true`.


http => HTTP

I think the sentence could be clearer about what it actually does.

sindresorhus · 2021-05-04T12:59:38Z

readme.md

+isAbsoluteUrl('https://si', true);
+//=> true
+
+isabsoluteUrl('mailto:[email protected]', false);


sindresorhus · 2021-05-04T12:59:48Z

readme.md

+
+isabsoluteUrl('mailto:[email protected]', false);
+//=> true
+


Trailing whitespace

sindresorhus · 2021-05-04T13:00:26Z

test.js

@@ -17,3 +17,61 @@ test('main', t => {
 	t.false(isAbsoluteUrl('C:\\Dev\\test-broken'));
 	t.false(isAbsoluteUrl('ht,tp://sindresorhus.com'));
 });
+
+test('HttpOnly set as true (same as default behavior). Assert TRUE tests', t => {


Too verbose test title. Formatting needs work too.

sindresorhus · 2021-05-04T13:01:04Z

test.js

+	// eslint-disable-line no-script-url
+	t.true(isAbsoluteUrl('javascript:throw%20document.cookie', false));
+
+	t.true(isAbsoluteUrl('https://sindresorhus', false));	// Should this behavior exist? missing TLD?...


Instead of asking questions, try to research and figure it out.

I felt it should at least check for the TLD, but it would return false to URLs as http://localhost:7700 and they are absolute. So it will not check for it.

…nd d.ts

sindresorhus · 2021-05-05T15:43:10Z

index.js

@@ -1,10 +1,14 @@
 'use strict';

-module.exports = url => {
+module.exports = (url, options = {httpOnly: true}) => {


This is not the correct way to do default values for options objects. If another option is added, it will silently not do the right thing as both options will be overwritten if one is set.

Suggested change

module.exports = (url, options = {httpOnly: true}) => {

module.exports = (url, {httpOnly = true} = {}) => {

sindresorhus · 2021-05-05T15:43:53Z

index.js

 	if (typeof url !== 'string') {
 		throw new TypeError(`Expected a \`string\`, got \`${typeof url}\``);
 	}

+	if (options.httpOnly) {
+		return /^((http[s]?)?:\/\/([-a-z0-9()@:%_.~#?&//=]*))/gi.test(url);


The regex could still be simplified.

readme.md

Co-authored-by: Sindre Sorhus <[email protected]>

JoaoOliveiraRocha added 6 commits April 28, 2021 20:11

Found problems with XO and Chaining operator

cbd5a81

Done 3 levels of Strictness and added tests for them

4da98fa

Added tests for a bunch of weird cases

cc50c0a

Created a test to show the advantage of httpOnly

d0771b5

Changed the Readme.md

627c1e9

removed useless comments

65e9aec

sindresorhus reviewed Apr 29, 2021

View reviewed changes

readme.md Outdated

```

Copy link

Owner

sindresorhus Apr 29, 2021

Choose a reason for hiding this comment

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

Don't do unrelated changes. Applies everywhere.

satazor reacted with thumbs up emoji

sindresorhus reviewed Apr 29, 2021

View reviewed changes

kept the inital readme.md

8af5eff

satazor suggested changes Apr 29, 2021

View reviewed changes

satazor reviewed Apr 29, 2021

View reviewed changes

JoaoOliveiraRocha added 4 commits April 29, 2021 19:05

Adding the suggested changes

046d49a

added declarations

c6f55e1

added declarations

488c69d

improved declaration on index.test-d.ts

f7854da

sindresorhus requested changes May 2, 2021

View reviewed changes

JoaoOliveiraRocha added 2 commits May 2, 2021 16:20

Simpler solution

e038281

Fixed XO warnings

ad5772a

JoaoOliveiraRocha changed the title ~~Attempt to solve #11 while maintaining the current usage~~ Add HTTP URLs verification by default May 2, 2021

Added test for whitespaces

d721ee8

sindresorhus requested changes May 4, 2021

View reviewed changes

Done regex simplification and test. Still need to fix documentation a…

4162a55

…nd d.ts

JoaoOliveiraRocha and others added 4 commits May 5, 2021 11:17

Changed back to parameter object

010ef03

Changed test description

cef4b54

added test to 'http://localhost'

f8b0260

Update index.test-d.ts

7d5cbb2

sindresorhus requested changes May 5, 2021

View reviewed changes

Update readme.md

8d340dd

Co-authored-by: Sindre Sorhus <[email protected]>

sindresorhus closed this Aug 10, 2021

sindresorhus mentioned this pull request Aug 10, 2021

Only allow HTTP URLs by default #11

Open


		See [is-relative-url](https://github.com/sindresorhus/is-relative-url) for the inverse.
		### httpOnly

		@param url - The URL to check.
		@param options - Object with different parameters to check.

	expectType<boolean>(isAbsoluteUrl('https://sindresorhus.com/foo/bar', { httpOnly: true }));
	expectType<boolean>(isAbsoluteUrl('https://sindresorhus.com/foo/bar', {httpOnly: true}));

	module.exports = (url, options = {httpOnly: true}) => {
	module.exports = (url, {httpOnly = true} = {}) => {

Add HTTP URLs verification by default #14

Add HTTP URLs verification by default #14

Conversation

JoaoOliveiraRocha commented Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Apr 29, 2021

sindresorhus commented Apr 29, 2021

JoaoOliveiraRocha commented Apr 29, 2021

satazor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented May 2, 2021

sindresorhus commented May 2, 2021

sindresorhus commented May 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented May 2, 2021

JoaoOliveiraRocha commented May 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoOliveiraRocha commented Apr 28, 2021 •

edited

Loading