Skip to content

Jc/fix code smells #84

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

Open
johnpcooke94 wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Jc/fix code smells #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
69d88f5
Remove redundant import
johnpcooke94 Nov 14, 2021
9dc48c1
Fix code smells - redundant awaits, redundant type casts, tslint issues
johnpcooke94 Nov 14, 2021
80b2efd
Merge branch 'dev' into jc/fix-code-smells
johnpcooke94 Nov 21, 2021
b97b8e4
Add a comment for PR
johnpcooke94 Nov 21, 2021
8b41a2c
Better fix for file upload code smell
johnpcooke94 Jan 23, 2022
6d87a30
Merge branch 'dev' into jc/fix-code-smells
johnpcooke94 Jan 23, 2022
0cef15f
Restore linter ignore for redux to work
johnpcooke94 Jan 23, 2022
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
3 changes: 1 addition & 2 deletions src/components/AdminPanelLayout/UsersPane.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import {Button, Form, InputGroup, Modal, ModalBody, ModalFooter, Table} from 'react-bootstrap';
import React, {useState} from 'react';
import {Role, User} from '../../types/User';
import {changeUserRole, getCurrentUserFromServer, getUserList} from '../../api';
import {changeUserRole, ChangeUserRoleRequest, getCurrentUserFromServer, getUserList} from '../../api';
import {useMount} from '../../hooks/useMount';
import {HorizontallyCenteredSpinner} from '../util/HorizonallyCenteredSpinner';
import {Layout} from '../../pages/Layout/Layout';
import UserListItem from './UserListItem';
import {ChangeUserRoleRequest} from '../../api';
import {AxiosResponse} from 'axios';
import ModalHeader from 'react-bootstrap/ModalHeader';
import {connect, useDispatch} from 'react-redux';
Expand Down
1 change: 1 addition & 0 deletions src/components/ConsoleWindow/ConsolePopout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class ConsolePopout extends Component<Props> {

this.externalWindow!.document.title = 'VM Popout';

// Sonarcloud thinks this line is a security issue
this.externalWindow!.addEventListener('beforeunload', (ev: BeforeUnloadEvent) => {

@johnpcooke94 johnpcooke94 Nov 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jasekiw @Cliftonz Sonarcloud thinks this is a security issue because we aren't verifying the origin of the message that we're listening to. But this is a reference to a window that we opened ourselves, so I don't think there's any real security risk here? I could be wrong, just wanted to check before I marked the issue resolved on Sonarcloud.

@Cliftonz Cliftonz Nov 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Someone could intercept our call back to a users and put their own vnc messages in there.
I remember talking about this with Jason but I do not remember what we decided on.

@johnpcooke94 johnpcooke94 Nov 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Normally the solution is to just verify the event origin, which I tried, but the BeforeUnloadEvent type that we're using doesn't seem to have a origin property. So I'm not sure what the proper solution here would be, at least not without some significant refactoring.

Let's see if Jason has any ideas, he built this component.

@jasekiw jasekiw Dec 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Cliftonz How could someone intercept our callback? They would have to do cross site scripting which would be an even worse exploit.

@jasekiw jasekiw Dec 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Cliftonz We're talking about an event handler that asks the user if they are sure if they want to exit, how much damage could exploiting this method really do?

@johnpcooke94 johnpcooke94 Jan 23, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jasekiw I'm with you here, I think. I can mark this as won't fix if you're okay with that.

@jasekiw jasekiw Apr 5, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we are ok with this

if(!this.shouldAsk) {
return;
Expand Down
4 changes: 2 additions & 2 deletions src/components/VmTemplateModal/VmTemplateUploadSchema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {object, ObjectSchema, ObjectSchemaDefinition, string} from 'yup';
import {object, ObjectSchemaDefinition, string} from 'yup';
import {cast} from '../../util';

export interface NamedUpload {
Expand All @@ -16,7 +16,7 @@ export interface UploadByUrlForm extends NamedUpload {
const NameSchema: ObjectSchemaDefinition<NamedUpload> = {name: string().required('Required').min(3, 'Must have at least 3 characters')};
const UploadFormSchema = object<UploadForm>({
...NameSchema,
file: object().nullable() as ObjectSchema<File>
file: object<File>().nullable().required('File is required')
});

export const isUploadForm = (val: NamedUpload): val is UploadForm => val['file'] !== undefined;
Expand Down
5 changes: 2 additions & 3 deletions src/pages/Contact/ContactUs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ export default function ContactUs() {
formData.append('message', form.message);
const files = form.screenshots;
if (files !== null) {
// tslint:disable-next-line:prefer-for-of
for (let i = 0; i < files.length; i++) {
formData.append('screenshots', files[i]);
for (const file of Array.from(files)) {
formData.append('screenshots', file);
}
}
await submitContactRequest(formData);
Expand Down
2 changes: 1 addition & 1 deletion src/pages/LoginRegisterPage/RegisterFormSchema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,6 @@ export const RegisterFormSchema: ObjectSchema<RegisterFormValues> = object({
password: passwordValidator,
confirmPass: makeConfirmPasswordValidator('password'),
gradYear: string(),
phoneNumber: string().matches(/[0-9]{3}-[0-9]{3}-[0-9]{4}/, 'Please type in format of XXX-XXX-XXXX'),
phoneNumber: string().matches(/\d{3}-\d{3}-\d{4}/, 'Please type in format of XXX-XXX-XXXX'),
acceptedTerms: bool().test('accepted-terms', 'You must agree before submitting.', (value?: boolean) => Boolean(value))
});
1 change: 0 additions & 1 deletion src/redux/reducers/browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {BrowserTypes} from '../types/actionTypes';
import {WindowState} from '../types/BrowserState';
import Action from '../types/redux';

// Code smell on this function signature
function windowSize(state: WindowState = {height: NaN, width: NaN}, action: Action) {

@jasekiw jasekiw Dec 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is how redux is designed to be used, this code smell needs to be disabled

@Cliftonz Cliftonz Dec 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

from what I see it is just saying to put the defaults at the end.
https://sonarcloud.io/project/issues?id=ius-csg_cslabs-webapp&issues=AXsiTV_Kw3kkXW2HHOS-&open=AXsiTV_Kw3kkXW2HHOS-

@johnpcooke94 johnpcooke94 Jan 23, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Jason is right, if you try to flip it, it breaks redux. I marked this as a false positive.

if (action.type === BrowserTypes.SCREEN_RESIZE) {
return {
Expand Down
3 changes: 1 addition & 2 deletions src/redux/store/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,16 @@ import {persistGlobalStore, persistRootReducer} from './persistance';

const configureStore = (initialState?: DeepPartial<WebState>, onReady?: () => void) => {
const root = persistRootReducer(combineReducers(rootReducer(history)));
// @ts-ignore

@jasekiw jasekiw Dec 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This used to break without ts-ignore, it doesn't any more?

@johnpcooke94 johnpcooke94 Jan 23, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, it was working when I opened this PR. But now I've merged dev in and the linter is mad, so I'm not sure what changed. Either way I'll just put it back.

const composeEnhancers: typeof compose = (typeof window !== 'undefined' && window['__REDUX_DEVTOOLS_EXTENSION_COMPOSE__']) || compose;
const storeInstance = createStore(
root,
initialState,
// Code smell on this

@jasekiw jasekiw Dec 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is completely false, it does change the type expressions and is necessary

@johnpcooke94 johnpcooke94 Jan 23, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's why I removed this comment. I was using the comments to help me keep track of what code smells I hadn't fixed yet. Once I realized this was a false positive I removed the comment and marked the smell resolved.

composeEnhancers(applyMiddleware(thunk as ThunkMiddleware<WebState, AnyAction>, routerMiddleware(history)))
);
const persistorInstance = persistGlobalStore(storeInstance, onReady);
storeInstance.dispatch(initBrowser());
return { store: storeInstance, persistor: persistorInstance, appDispatch: storeInstance.dispatch};
};

// @ts-ignore
export const {store, persistor, appDispatch } = configureStore();